Add max code length tests, fix behavior for Python, Rust and C++ #300
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WilliamDenniss
changed the title
WilliamDenniss
force-pushed
the
maxlength
branch
from
775daa2 to
4fc7378
Compare
WilliamDenniss
force-pushed
the
maxlength
branch
from
4fc7378 to
c001f45
Compare
|
The C++ library seems to be failing the new tests. If you have time, would you mind looking into it? If not, I can take a look next week. |
WilliamDenniss
force-pushed
the
maxlength
branch
3 times, most recently
from
38f46c7 to
66a4636
Compare
WilliamDenniss
changed the title
|
Sure thing, I fixed the C++ issue (was returning the original size in the CodeArea object) by moving the truncation a bit earlier. |
WilliamDenniss
force-pushed
the
maxlength
branch
2 times, most recently
from
ab80f24 to
b9976bf
Compare
|
@zongweil I believe this is good to go, all implementations are now passing the tests I added to verify the max code length behavior. |
zongweil
requested changes
Apr 26, 2019
cpp/openlocationcode.cc
Outdated
| @@ -199,7 +199,7 @@ std::string Encode(const LatLng &location) { | |||
|
|
|||
| CodeArea Decode(const std::string &code) { | |||
| // Make a copy that doesn't have the separator and stops at the first padding | |||
| // character. | |||
| // character, and is constrained to the maximum length. | |||
| std::string clean_code(code); | |||
| clean_code.erase( | |||
There was a problem hiding this comment.
The code from lines 203-210 seems to be duplicated in CodeLength(). Can you extract and de-dupe?
There was a problem hiding this comment.
Sure thing! Done.
WilliamDenniss
force-pushed
the
maxlength
branch
from
9bd842d to
1e6891e
Compare
As documented in issue number 190: 1. If you ask the library to encode a lat/long with more than 15 digits, the library will return a plus code with 15 digits. 2. If you ask the library to decode a plus code with more than 15 digits, the library will only take the first 15 digits into account with the returned code area. 3. Plus codes with more than 15 digits will be treated as valid if all the other validity constraints are satisfied. If the plus code has an invalid character after the 15th digit, it will be treated as invalid.
Limit decoding to first 15 significant digits.
Decode was only processing the first 15 significant digits, but was returning the length of the entire code. Refactored a little to move the truncation earlier.
WilliamDenniss
force-pushed
the
maxlength
branch
from
1e6891e to
76b869c
Compare
|
@zongweil this should be ready now, PTAL :) |
zongweil
approved these changes
Apr 29, 2019
cpp/openlocationcode.cc
Outdated
| @@ -92,6 +92,19 @@ double adjust_latitude(double latitude_degrees, size_t code_length) { | |||
| return latitude_degrees - precision / 2; | |||
| } | |||
|
|
|||
| // Remove the separateor and padding characters from the code. | |||
|
Thanks! One typo, I'll merge it in right after :). |
WilliamDenniss
force-pushed
the
maxlength
branch
from
76b869c to
af246f0
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replaces #287, moves tests to data files.
Includes #299 (I will rebase when that is merged).