-
-
Notifications
You must be signed in to change notification settings - Fork 87
ENH: use attribution from provider if available #91
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
ENH: use attribution from provider if available #91
Conversation
|
This looks good to me. I'd only say maybe, if we bring in #94 it should be included here too? |
Not fully sure what you mean? We will merge both PRs, and depending on which is merged first, the other will need to fix the conflicts? |
| if url is None: | ||
| url = providers.Stamen.Terrain | ||
| if isinstance(url, dict) and attribution is None: | ||
| attribution = url.get("attribution") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a little bit unfortunate that we need to do the above in more than one place, but I don't think this is avoidable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we maybe turn it into a method that we can use throughout? Something like:
def check_attribution(url, attribution):
if url is None:
url = providers.Stamen.Terrain
if isinstance(url, dict) and attribution is None:
attribution = url.get("attribution")
return url, attributionThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now it is only in 2 places. So maybe let's factor out once there is a third ;)
darribas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just very minor changes. The only other thing I'd say it'd be really great to have too is a short notebook demonstrating the providers functionality and how it's embedded in add_basemap, etc.
| axis_off=True, | ||
| latlon=True, | ||
| attribution=ATTRIBUTION, | ||
| place, bbox=None, title=None, ax=None, axis_off=True, latlon=True, attribution=None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not black compatible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's black that did it, so I suppose yes :)
(I think the rule is: if it fits on one line, use one line, if not, put each keyword on its own line)
| if url is None: | ||
| url = providers.Stamen.Terrain | ||
| if isinstance(url, dict) and attribution is None: | ||
| attribution = url.get("attribution") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we maybe turn it into a method that we can use throughout? Something like:
def check_attribution(url, attribution):
if url is None:
url = providers.Stamen.Terrain
if isinstance(url, dict) and attribution is None:
attribution = url.get("attribution")
return url, attribution| att : str | ||
| [Optional. Defaults to standard `ATTRIBUTION`] Text to be added at the | ||
| bottom of the axis. | ||
| text : str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this renaming, makes more sense
|
OK, given the minor comments are somewhat answered, will merge this now |
Work in progress, still needs to add tests and update docs. And was trying out if I could "auto wrap" the text, but not yet much success (https://matplotlib.org/3.1.1/gallery/text_labels_and_annotations/autowrap.html)