Skip to content
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

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

Open
esuomi opened this issue Oct 16, 2023 · 4 comments
Labels
enhancement New feature or request

Comments

@esuomi
Copy link

esuomi commented Oct 16, 2023

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
Copy link
Contributor

richfab commented Oct 18, 2023

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!

@richfab richfab added the enhancement New feature or request label Oct 18, 2023
@davidgamez
Copy link
Member

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
Copy link
Author

esuomi commented Feb 8, 2024

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants