added get_bounds functions for EPSG 4326#980
Conversation
spjuhel
left a comment
spjuhel
left a comment
There was a problem hiding this comment.
This looks good overall,
I made some minor suggestions, you may choose to ignore (except maybe the additional tests).
There was a problem hiding this comment.
You should add tests for invalid bounding box.
Co-authored-by: Samuel Juhel <10011382+spjuhel@users.noreply.github.com>
There was a problem hiding this comment.
wouldn't it be better to simply throw an exception in case the country is not recognized?
I can't see the point of filtering out the unrecognizable countries in here. If a user runs this with a typo in one of the country names would they be upset if it fails?
There was a problem hiding this comment.
Yes I agree, this would be better. I changed it to raise a ValueError now.
My only doubt is that the function util.coordinates.get_country_geometries() existed before and if you input a list of ISO codes with one invalid code, it did NOT raise an error before (this is why I "only" added a warning). Wouldn't this be a potential problem for existing code to break down due to the error, or is it better to make aware that the input was not correct?
There was a problem hiding this comment.
Good question. Imo, it is better to make them finally aware that their input has been wrong all the time.
We just need to add a note in the change log.
There was a problem hiding this comment.
Perfect, agreed. I'm adapting the change log accordingly.
3 participants
Changes proposed in this PR:
bounding_box_global,bounding_box_from_countries, andbounding_box_from_cardinal_boundsin theclimada.util.coordinatesmodule.Notes
PR Author Checklist
develop)PR Reviewer Checklist