-
Notifications
You must be signed in to change notification settings - Fork 291
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
Syncs systems.csv with transport.data.gouv #593
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
2acf19c
Syncs systems.csv with transport.data.gouv
richfab ac80914
Move public keys to Authentication Info column
richfab 569efb6
Update Authentication Info description in README
richfab ad39f8e
Rephrase Authentication Info description in README
richfab File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 wouldn't be a key-value pair. I think what you want to say here is "contain the API key or token to append to the feed URLs"
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.
Thanks. How can we make explicit that the name of the parameter is required too?
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 think this is getting overly complicated - I don't see any reason to separate the key from the URL in the auto-discovery column. What's the value in that? We've got lots of URLS with parameters for example:
https://qickscooters.rideatom.com/gbfs/v2_2/en/gbfs?id=104the only difference is that in this case it's an API key not asystem_idbut they function in the same way since the API key doesn't change. It's my understanding that the last column was added to support Auth schemes that use tokens or other forms of Auth that are subject to change and therefore needed a URL to provide additional info on how to get access.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.
The problem here is that
gbfs.jsonreferences the sub-feeds without the key. It needs to be appended manually to them. So I suggest keeping the "key value pair" language.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'm sorry - I misunderstood the problem. Ortwin is correct. I didn't realize that the URLS in
gbfs.jsonlacked the key. However I would also argue that this is broken becausefeeds[].urlingbfs.jsonshould contain a valid URL pointing to the file. Maybe we should consider updating that definition and requiring valid URLs. I've seen this lots of times the past and I just left the feeds out ofsystems.csvuntil they fixed thegbfs.jsonfile.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 both for your input!
I agree. I informed the producer that it would be best to remove the public key or append it to all URLs in
gbfs.json.+1