-
Notifications
You must be signed in to change notification settings - Fork 213
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add map_id parameter to static map request #270
feat: add map_id parameter to static map request #270
Conversation
@@ -37,6 +37,7 @@ var ( | |||
scale = flag.Int("scale", -1, "Scale affects the number of pixels that are returned.") | |||
format = flag.String("format", "", "Format defines the format of the resulting image.") | |||
maptype = flag.String("maptype", "", "Maptype defines the type of map to construct.") | |||
mapid = flag.String("mapid", "", "MapId defines the mapid to use.") |
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.
Naming convention for the majority of these is to exclude the map prefix even though they are map values. I think it would make sense and read better to use just id
instead of mapi
.
It would also be nice for the description to talk about what id
does rather than just letting us know it's a 1:1 pass through to the API. Maybe this line taken from the API docs: associates a map with a particular style or feature
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 forgot this PR was still open...
I followed the existing code convention. See maptype above. Besides, it's the name of the variable in the request in the static maps API docs. I didn't want to take the liberty of changing variable names given all the others are 1:1 matches.
As for the description, I figured anyone using this library would refer to the API docs if something is unclear.
I'm also interested in adding this parameter. I looked over the PR and gave my 0.02$ if that helps merge the change at all. |
Please merge |
馃帀 This PR is included in version 1.5.0 馃帀 The release is available on GitHub release Your semantic-release bot 馃摝馃殌 |
Add support for map_id parameter to static map request and static map cli app
Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #269 馃