-
Notifications
You must be signed in to change notification settings - Fork 660
Added ability to use local ArcGIS server #325
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
Conversation
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.
Thank you so much! I left some feedback below which require some minor changes. Otherwise the change looks good to me 😊
geopy/geocoders/arcgis.py
Outdated
:param str domain: Domain where the target ArcGIS service | ||
is hosted. | ||
|
||
:param str auth_domain: Domain where the target ArcGIS auth service |
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.
Order of params in the docstring should match the order of arguments of the function.
Either function args or the params in the doc should be swapped.
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.
Also I would suggest to add to the auth_domain doc a clarification that this parameter will be used only when authentication is enabled (i.e. username, password or referer are set), otherwise it might be confusing what to put here if a local geocoder server does not have a corresponding auth server.
Linking #192 — the original issue. |
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.
Thank you! Merging
No description provided.