feat: Enhance cli command line #107

davidgamez · 2023-06-29T20:00:14Z

Description

In this PR, the cli.js file is enhanced with additional parameters, help messages, and error handling. The following parameters were added:

Usage: cli [OPTIONS]...

Options:
  -v, --version                    output the version number
  -u, --url <feed_url>          URL of the GBFS feed
  -vb, --verbose                   Verbose mode prints debugging console logs
  -s, --save-report <report_path>  Local path to output report file
  -pr, --print-report <yes_no>     Print report to standard output (choices: "yes", "no", default: "yes")
  -h, --help                       display help for command

#Tasks pending before Ready

add unit tests
Update REAME.md

netlify · 2023-06-29T20:00:18Z

✅ Deploy Preview for gbfs-validator ready!

Name	Link
🔨 Latest commit	`cadabd5`
🔍 Latest deploy log	https://app.netlify.com/sites/gbfs-validator/deploys/64a5b694bf0f7c0008c73443
😎 Deploy Preview	https://deploy-preview-107--gbfs-validator.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

PierrickP

Thanks for this PR, it's a great improvement.
I made 2 personal remarks, and maybe we can add a "summary" option to only have the summary ?

PierrickP · 2023-07-04T07:50:21Z

gbfs-validator/cli.js

-const gbfs = new GBFS(process.argv[2])
+parseOptions = () => {
+  commander
+    .version(pjson.version, '-v, --version')


It's always 1.0.0 because we don't use it right now.
We return the git hash instead https://github.com/MobilityData/gbfs-validator/blob/master/gbfs-validator/gbfs.js#L3

This is in preparation for publishing an npm package. In the case of the command line, if we would like to include the commit hash, more work is needed to make available the hash information on local computers.

PierrickP · 2023-07-04T07:54:15Z

gbfs-validator/cli.js

+  }
+}
+
+if (options.url) {


As this parameter is mandatory, it may be preferable to pass the URL such as node gbfs-validator/cli.js [URL] instead of -u.

Good point, I'll add the option as mandatory. I considered adding -f for local files in the near future; that's why defining -u for URLs and -f for files gives consistency for the consumer.

Interesting for the local files :)

PierrickP · 2023-07-04T07:56:32Z

gbfs-validator/cli.js

+  }
+  try {
+    const report = await getFeedValidationReport(options.url)
+    if (options.print) {


Maybe make it default ?
If no -p / -s arguments is passed, nothing happen, and we don't have any clue

What do you think changing -p to -nstdout or similar as No standard output, in addition to trigger an error if -nstdout is passed and -s is not present.

Alternatively, what do you think about removing the -p option completely and only print when option -s is missing?

That's a good solution too. My only concern is to drive the use of the command line too much. Having separate options give more control over what goes to a file and stdout.

gbfs-validator/cli.js

…to a cli like testing

davidgamez · 2023-07-04T20:21:04Z

Thanks for this PR, it's a great improvement. I made 2 personal remarks, and maybe we can add a "summary" option to only have the summary?

Hi @PierrickP, can you expand on what information might be interesting to have in the summary response? I might defer this task to a follow-up PR.

PierrickP · 2023-07-05T07:16:36Z

@davidgamez On summary (based on #101) i would like if the feed is valid or not, versions, and the number of errors (distinct and total, part of #106).

davidgamez · 2023-07-05T10:42:20Z

@davidgamez On summary (based on #101) i would like if the feed is valid or not, versions, and the number of errors (distinct and total, part of #106).

@PierrickP, thanks for the response. I'll have this as a follow-up task after the mentioned PRs are reviewed and merged.

gbfs-validator/cli.js

gbfs-validator/__test__/cli.test.js

gbfs-validator/cli.js

gbfs-validator/__test__/cli.test.js

Co-authored-by: Fabien Richard-Allouard <fabien@mobilitydata.org>

richfab · 2023-07-05T18:57:08Z

gbfs-validator/cli.js

+    .requiredOption('-u, --url <feed_url>', 'URL of the GBFS feed')
+    .option('-vb, --verbose', 'Verbose mode prints debugging console logs')
+    .option('-s, --save-report <report_path>', 'Local path to output report file')
+    .addOption(new commander.Option('-pr, --print-report <yes_no>', 'Print report to standard output').default('yes').choices(['yes', 'no']))


For consistency with -s, could we use one letter -p for printing?

Suggested change

.addOption(new commander.Option('-pr, --print-report <yes_no>', 'Print report to standard output').default('yes').choices(['yes', 'no']))

.addOption(new commander.Option('-p, --print-report <yes_no>', 'Print report to standard output').default('yes').choices(['yes', 'no']))

I connected with @richfab, and we couldn't find any strong feelings either way. Let's keep it as it is for simplicity; in the future, more options will be added, and the short argument name will be always a point of conflict.

richfab · 2023-07-05T18:58:33Z

gbfs-validator/__test__/cli.test.js

-      const url = `http://${gbfsFeedServer.server.address().address}:${
-        gbfsFeedServer.server.address().port
-      }`
+  test('should success and print the report when url parameter set and -pr is set as default', async () => {


(Only if you accept the suggestion to use -p for consistency with -s)

Suggested change

test('should success and print the report when url parameter set and -pr is set as default', async () => {

test('should success and print the report when url parameter set and -p is set as default', async () => {

richfab · 2023-07-05T18:58:51Z

gbfs-validator/__test__/cli.test.js


-      process.argv[2] = url
+  test('should show an error if pr parameter is set to `no` and -s is not set', async () => {
+    const result = await cliExec([`-u ${feedUrl}`, '-pr no'])


(Only if you accept the suggestion to use -p for consistency with -s)

Suggested change

const result = await cliExec([`-u ${feedUrl}`, '-pr no'])

const result = await cliExec([`-u ${feedUrl}`, '-p no'])

richfab · 2023-07-05T18:59:09Z

gbfs-validator/__test__/cli.test.js


-      const cli = await require('../cli.js')
+  test('should success when paramters url and save report has valid values and print report is set to no', async () => {
+    const result = await cliExec([`-u ${feedUrl}`, '-s /dev/null', '-pr no'])


(Only if you accept the suggestion to use -p for consistency with -s)

Suggested change

const result = await cliExec([`-u ${feedUrl}`, '-s /dev/null', '-pr no'])

const result = await cliExec([`-u ${feedUrl}`, '-s /dev/null', '-p no'])

richfab · 2023-07-05T18:59:22Z

gbfs-validator/__test__/cli.test.js

-      })
-    })
+  test('should success and print report when paramters url and save report are valid and print report is set to yes', async () => {
+    const result = await cliExec([`-u ${feedUrl}`, '-s /dev/null', '-pr yes'])


(Only if you accept the suggestion to use -p for consistency with -s)

Suggested change

const result = await cliExec([`-u ${feedUrl}`, '-s /dev/null', '-pr yes'])

const result = await cliExec([`-u ${feedUrl}`, '-s /dev/null', '-p yes'])

richfab · 2023-07-05T19:04:17Z

README.md

+  -u, --url <feed_url>             URL of the GBFS feed
+  -vb, --verbose                   Verbose mode prints debugging console logs
+  -s, --save-report <report_path>  Local path to output report file
+  -pr, --print-report <yes_no>     Print report to standard output (choices: "yes", "no", default: "yes")


(Only if you accept the suggestion to use -p for consistency with -s)

Suggested change

-pr, --print-report <yes_no> Print report to standard output (choices: "yes", "no", default: "yes")

-p, --print-report <yes_no> Print report to standard output (choices: "yes", "no", default: "yes")

richfab

LGTM.

Thank you @davidgamez for this great improvement to the CLI tool!

enhance cli command line

9022da9

fix url parameter help comment

726a228

PierrickP reviewed Jul 4, 2023

View reviewed changes

add more unit test to cli

f00a08e

emmambd requested a review from richfab July 4, 2023 14:50

emmambd assigned richfab Jul 4, 2023

richfab reviewed Jul 4, 2023

View reviewed changes

gbfs-validator/cli.js Outdated Show resolved Hide resolved

refactor unit tests reducing the amount of mocking and aligning them …

cb171c7

…to a cli like testing

davidgamez added 2 commits July 4, 2023 16:37

update README file

7d62465

rename -u parameter description from dataset_url to feed_url

998ca5e

davidgamez marked this pull request as ready for review July 4, 2023 20:41

add missing unit test fix

f290505

davidgamez requested review from PierrickP and richfab July 4, 2023 20:43

davidgamez added 2 commits July 4, 2023 16:47

unit test clean up

aa288b9

formatting

3e5a7ea

PierrickP approved these changes Jul 5, 2023

View reviewed changes

davidgamez mentioned this pull request Jul 5, 2023

Add summary option to cli #112

Open

richfab reviewed Jul 5, 2023

View reviewed changes

gbfs-validator/cli.js Outdated Show resolved Hide resolved

set feed_url as mandatory value

9d6d134

github-advanced-security bot found potential problems Jul 5, 2023

View reviewed changes

gbfs-validator/__test__/cli.test.js Dismissed Show dismissed Hide dismissed

richfab reviewed Jul 5, 2023

View reviewed changes

gbfs-validator/cli.js Outdated Show resolved Hide resolved

richfab reviewed Jul 5, 2023

View reviewed changes

gbfs-validator/__test__/cli.test.js Outdated Show resolved Hide resolved

davidgamez and others added 2 commits July 5, 2023 14:29

Update gbfs-validator/cli.js

aecc925

Co-authored-by: Fabien Richard-Allouard <fabien@mobilitydata.org>

Update gbfs-validator/__test__/cli.test.js

cadabd5

Co-authored-by: Fabien Richard-Allouard <fabien@mobilitydata.org>

richfab reviewed Jul 5, 2023

View reviewed changes

richfab approved these changes Jul 5, 2023

View reviewed changes

davidgamez merged commit 2bc59f9 into master Jul 5, 2023
8 checks passed

davidgamez deleted the feat/enhance-cli-command branch July 5, 2023 20:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Enhance cli command line #107

feat: Enhance cli command line #107

davidgamez commented Jun 29, 2023 •

edited

netlify bot commented Jun 29, 2023 •

edited

PierrickP left a comment

PierrickP Jul 4, 2023

davidgamez Jul 4, 2023

PierrickP Jul 4, 2023

davidgamez Jul 4, 2023

PierrickP Jul 4, 2023

PierrickP Jul 4, 2023

davidgamez Jul 4, 2023

richfab Jul 4, 2023

davidgamez Jul 4, 2023

davidgamez commented Jul 4, 2023

PierrickP commented Jul 5, 2023

davidgamez commented Jul 5, 2023

richfab Jul 5, 2023

davidgamez Jul 5, 2023

richfab Jul 5, 2023

richfab Jul 5, 2023

richfab Jul 5, 2023

richfab Jul 5, 2023

richfab Jul 5, 2023 •

edited

richfab left a comment

	.addOption(new commander.Option('-pr, --print-report <yes_no>', 'Print report to standard output').default('yes').choices(['yes', 'no']))
	.addOption(new commander.Option('-p, --print-report <yes_no>', 'Print report to standard output').default('yes').choices(['yes', 'no']))

	test('should success and print the report when url parameter set and -pr is set as default', async () => {
	test('should success and print the report when url parameter set and -p is set as default', async () => {

	const result = await cliExec([`-u ${feedUrl}`, '-pr no'])
	const result = await cliExec([`-u ${feedUrl}`, '-p no'])

	-pr, --print-report <yes_no> Print report to standard output (choices: "yes", "no", default: "yes")
	-p, --print-report <yes_no> Print report to standard output (choices: "yes", "no", default: "yes")

feat: Enhance cli command line #107

feat: Enhance cli command line #107

Conversation

davidgamez commented Jun 29, 2023 • edited

Description

netlify bot commented Jun 29, 2023 • edited

✅ Deploy Preview for gbfs-validator ready!

PierrickP left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidgamez commented Jul 4, 2023

PierrickP commented Jul 5, 2023

davidgamez commented Jul 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richfab Jul 5, 2023 • edited

Choose a reason for hiding this comment

richfab left a comment

Choose a reason for hiding this comment

davidgamez commented Jun 29, 2023 •

edited

netlify bot commented Jun 29, 2023 •

edited

richfab Jul 5, 2023 •

edited