-
Notifications
You must be signed in to change notification settings - Fork 82
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
geocoding_ban/ : French Geocoding with BAN #45
base: master
Are you sure you want to change the base?
Conversation
J'ai l'impression que tu as commité 2 plugins sur cette PR @Tristramg |
Oups, oui, effectivement, merci ! C’set corrigé ! |
Hi, Thanks for this contribution! Ever since BAN / BANO appeared, I had wanted to add this kind of features to DSS. First of all, do you confirm that you wish us to publish this plugin? A few things that I noticed:
I'll open a review for minor nitpicks, thanks again! |
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.
"name": "columns", | ||
"label" : "Address columns", | ||
"type": "COLUMNS", | ||
"description":"Multiple columns will be concatenated", |
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.
Add "columnRole": "input" (name of an input role), and you'll have autocompletion on the column names. Same for postcode and citycode.
"name": "post_code", | ||
"label" : "Column of the postcode", | ||
"type": "COLUMN", | ||
"description":"Only results of that postcode will be returned.", |
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.
You might want to state explicitely that this is optional
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.
You mean in the text description? not only "mandatory": false
?
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.
Yes. Currently, we don't really do a good job at showing visual hints from the "mandatory" field, so we tend to be explicit in descriptions.
"type": "INT", | ||
"defaultValue": 1000, | ||
"mandatory": true, | ||
"description": "Sending multiple requests in one iteration saves time" |
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.
Since the name is "Lines per request", we might want to avoid "requests per iteration", and repeat line per request in the description. Since the API limits to 8MB (and since I guess returns are diminishing), it might be useful to state that you should generally not go above 1000
geocoding_ban/plugin.json
Outdated
{ | ||
// The identifier of the plugin. | ||
// This must be globally unique, and only contain A-Za-z0-9_- | ||
"id" : "geocoding_ban", |
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.
DSS plugins generally have names with dashes rather than underscore, so you might want to name that geocoding-ban
geocoding_ban/plugin.json
Outdated
// Meta data for display purposes | ||
"meta" : { | ||
// Name of this plugin that appears in the interface. | ||
"label": "BAN Geocoding plugin", |
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.
Suggestion: "French geocoding (BAN)"
Thank you for the review. Yes, we would like to publish this plugin. I also asked the people behind Addok and BANO and they are happy to know that there is a plugin.
Now a few questions:
|
Thanks!
|
Thank you. I hope I applied all your suggestions, and that I didn’t add some rubbish while doing so. |
🆙 ;) |
Ban is a French geocoding api http://adresse.data.gouv.fr/