Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement max code length behavior for Python #287

Closed
wants to merge 2 commits into from

Conversation

WilliamDenniss
Copy link
Contributor

Implements #190 for Python.

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.
@drinckes
Copy link
Contributor

Thanks William - I'd like to avoid having tests coded in implementations so that we have a common set of tests.

I realise that the current test files don't allow this, so I'm going to split the encoding and decoding tests into two files. (Issue #288, PR for the split and JS in #289.)

Once #289 is done, could you add your tests to that file, and then update the python tests to read from the new files?

I note that the JS test failed and I'll look into that.

@WilliamDenniss
Copy link
Contributor Author

Good plan! That's a much better approach.

I'll update this PR today to match, and can implement the split-test feature for Python as well.

@drinckes
Copy link
Contributor

I fixed the JS test (and a few others besides that had errors but weren't causing the builds to fail - it was just silently ignoring the errors).

@drinckes drinckes closed this Apr 24, 2019
@drinckes drinckes reopened this Apr 24, 2019
@drinckes
Copy link
Contributor

That button doesn't say cancel, does it? :-)

@zongweil
Copy link
Contributor

Doug, I was waiting for all implementations to make the max length changes before adding common test cases to the CSV (otherwise tests would fail). I took a look through the implementations and at this point, all the implementations referencing the CSV are actually updated, so we can finally consolidate.

@WilliamDenniss
Copy link
Contributor Author

Replaced by #300.

@zongweil good to know, will be looking to see if they all pass the tests I just added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants