MobilityData / gbfs-validator

The canonical GBFS validator. Maintained by the GBFS community, facilitated by MobilityData.
https://gbfs-validator.mobilitydata.org/
Apache License 2.0
18 stars 12 forks source link

Provide conditional local file based input/output parameters for CLI usage similar to the Canonical GTFS Validator #136

Open esuomi opened 11 months ago

esuomi commented 11 months ago

Hi, hello!

To keep this short: The current CLI implementation takes as an input parameter an URL (-u, --url) and set the location of its generated report with parameter -s, --save-report. This works, but has two issues:

  1. In a corporate network direct download from URLs can get blocked due to various reasons. It's also possible that the files being validated are not servable through an URL due to how the data is generated or otherwise shared among peers. Thus the ability to point to a local file is rather important.
  2. This is not symmetrical to Canonical GTFS Validator's parameters, namely its -i/--input and -o/--output. Symmetry is unsurprising, easy to predict and aesthetically pleasing :-)
richfab commented 11 months ago

Hello @esuomi and welcome!

  1. Being able to use a local file as input would be a great enhancement to the CLI. In this case, the parameter -i/--input could be created to match the Canonical GTFS Validator's parameters as @esuomi suggested. In the meantime, you may check out this workaround to validate a local feed: https://github.com/MobilityData/gbfs-validator/issues/70#issuecomment-1769122811.

  2. I don't see any issue for renaming the parameter -s/--save-report to -o/--output for symmetry with the Canonical GTFS Validator's parameters. @davidgamez what are your thoughts?

To summarize, the proposal is: Option Before After
Local path to output report file -s/--save-report -o/--output
URL of the GBFS feed -u/--url -u/--url (no change)
Local path to the GBFS feed (does not exist) -i/--input

Finally, @esuomi do you or anyone else have the resources to write the code for this feature?

Thank you very much for this contribution!

davidgamez commented 11 months ago

Hello @esuomi and welcome!

  1. Being able to use a local file as input would be a great enhancement to the CLI. In this case, the parameter -i/--input could be created to match the Canonical GTFS Validator's parameters as @esuomi suggested. In the meantime, you may check out this workaround to validate a local feed: CLI or http for local gbfs files #70 (comment).
  2. I don't see any issue for renaming the parameter -s/--save-report to -o/--output for symmetry with the Canonical GTFS Validator's parameters. @davidgamez what are your thoughts?

To summarize, the proposal is:

Option Before After Local path to output report file -s/--save-report -o/--output URL of the GBFS feed -u/--url -u/--url (no change) Local path to the GBFS feed (does not exist) -i/--input Finally, @esuomi do you or anyone else have the resources to write the code for this feature?

Thank you very much for this contribution!

I have no concerns about renaming the parameters; I suggest having a deprecation path in case there are consumers who already use the current names.

esuomi commented 7 months ago

Hi again!

So, I finally had a bit of time to come back to this and take an actual look into the codebase and related functionality just to get an idea of how difficult it would be to implement this.

While I'm not a JavaScript developer so I can't promise I'd personally contribute this feature, from what I can tell, the crux of this functionality is in gbfs.js#L374-L424. This also might make it a bit more difficult to implement this, as the URL-to-file resolving happens inside the GBFS validation logic?

I also briefly checked commander.js library used for command line options parsing and it doesn't seem to support alternative parameters out of the box, but probably has other functionalities for providing the "provide either --input or --url" kind of selection logic which this feature would need.

Anyway, I think a bit of agreement should first to be had on how this should be implemented, and there's still a possibility that while I personally wouldn't contribute this, we have our other team member who's more versed in JS/TS who maybe would be able to implement this. This is still not a hard promise though, wonders of bureaucracy and project planning do have their impact on such realities :)

davidgamez commented 7 months ago

Hi @esuomi,

While I'm not a JavaScript developer so I can't promise I'd personally contribute this feature, from what I can tell, the crux of this functionality is in gbfs.js#L374-L424. This also might make it a bit more difficult to implement this, as the URL-to-file resolving happens inside the GBFS validation logic?

You are right; the resolution of the files is tied to the validation logic(not ideal). I suggest implementing the changes in two phases/PRs.

  • Phase one is to get the URL content resolution outside the validation logic(the second phase will also resolve local files). This will need to take care of I/O exceptions in case URLs/files are not available. This will also serve as a verification of the base implementation.
  • In the second phase, add the parameter changes and file resolution. Even commander.js doesn't provide alternative parameters; we have an excellent spot to have this kind of verification, gbfs-validator/cli.js#L47

Currently, our hands are full with other priorities. If necessary, we can get into a call to support you and your team if you find spare time to work on this issue. Your taking the time to review the code and contribute means a lot to us.