Explicit coordinate validation #1090
Merged
Conversation
This file contains 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
stickler-ci
reviewed
Mar 10, 2019
Conengmo
force-pushed
the
explicit-coordinates
branch
from
9154e00
to
c961218
Compare
Conengmo
force-pushed
the
explicit-coordinates
branch
from
c961218
to
20fb4b1
Compare
Conengmo
force-pushed
the
explicit-coordinates
branch
2 times, most recently
from
a913a7d
to
f579392
Compare
ocefpaf
approved these changes
Mar 10, 2019
| from six.moves.urllib.parse import urlparse, uses_netloc, uses_params, uses_relative | ||
|
|
||
|
|
||
| _VALID_URLS = set(uses_relative + uses_netloc + uses_params) | ||
| _VALID_URLS.discard('') | ||
|
|
||
|
|
||
| def _is_sized_iterable(arg): | ||
| """Validates the arg is Sized & Iterable""" | ||
| return isinstance(arg, abc.Sized) & isinstance(arg, abc.Iterable) |
There was a problem hiding this comment.
Don't you think this was simpler with the abc? I'm fine with this approach but I could not see why the change at a first pass.
There was a problem hiding this comment.
Hmm I tried to change it into abc.Sequence, because it should also have __getitem__, but then it won't work for Numpy arrays. So my problem is that there may be other classes that don't inherit from abc types but have those methods implemented? I guess I prefer the try/except version because it's more intuitive and handles duck types better than abc.
Conengmo
force-pushed
the
explicit-coordinates
branch
from
f579392
to
2d31eeb
Compare
Conengmo
force-pushed
the
explicit-coordinates
branch
from
2d31eeb
to
64c9712
Compare
stickler-ci
reviewed
Mar 18, 2019
Conengmo
force-pushed
the
explicit-coordinates
branch
from
64c9712
to
635eb49
Compare
stickler-ci
reviewed
Mar 18, 2019
stickler-ci
reviewed
Mar 18, 2019
Conengmo
force-pushed
the
explicit-coordinates
branch
from
4711e3b
to
20b49ed
Compare
stickler-ci
reviewed
Mar 18, 2019
Conengmo
force-pushed
the
explicit-coordinates
branch
from
20b49ed
to
de6ea0a
Compare
Rewrite the validation functions in utilities that check single and multiple coordinate pairs. Make them more strict and raise meaningful exceptions. Apply the validation functions throughout the code base wherever possible. For the Marker class and vector classes a new BaseMarker class is added to make the distinction between single and multiple locations explicit. For the HeatMap and FastMarkerCluster classes I made custom validation in the classes themselves. Their use cases are different, so they cannot use the standard validation functions.
Conengmo
force-pushed
the
explicit-coordinates
branch
from
de6ea0a
to
05ca372
Compare
It contained an outdated use of the `MarkerCluster` class.
|
Hi @ocefpaf, I think this is pretty much ready. Could you check it out if you have a moment? |
|
Awesome changes @Conengmo! Thanks for fixing this!! |
|
Thanks for the review! |
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.
Problem
We have multiple conversion/validation functions in utilities. It's hard to decide which to use, and they are not thorough.
Solution
Have a few validation/conversion functions that are clear and explicit. They raise meaningful errors. They have extensive tests.
So that's what I tried in this PR. I put some things together into two functions: one for single coordinate pairs and one for lists (of lists) of coordinate pairs. Both try to raise meaningful exceptions and convert to a single common format.
I applied the validation functions throughout the code base when possible. In some cases (
HeatMap,FastMarkerCluster) I made custom ones because they have special data formats.For the Marker class and vector classes I added a new BaseMarker class to make the distinction between single and multiple locations explicit. Before the vector classes just passed a list of coordinate pairs to
Marker, whileMarkeris documented to only accept a single coordinate pair.Possible improvements:
@ocefpaf @jtbaker want to review this one?
Closes #1089