Skip to content

Adding a --validate CLI option#84

Closed
AVGP wants to merge 1 commit intojson5:masterfrom
AVGP:cli-validate
Closed

Adding a --validate CLI option#84
AVGP wants to merge 1 commit intojson5:masterfrom
AVGP:cli-validate

Conversation

@AVGP
Copy link
Copy Markdown

@AVGP AVGP commented Dec 23, 2014

Simple version of a --validate option for the CLI added by highjacking the -c option code.

Sort-of delivers #72

@ravi
Copy link
Copy Markdown

ravi commented Feb 9, 2015

Can we have this? (please!)

@AVGP
Copy link
Copy Markdown
Author

AVGP commented Feb 9, 2015

@ravi You can use my branch (referenced in this pull request) to try it :) I'm waiting for feedback or a merge on this, too

@aseemk
Copy link
Copy Markdown
Member

aseemk commented Feb 9, 2015

Whoops, I totally missed this PR, sorry about that! Taking a quick look now.

@ravi
Copy link
Copy Markdown

ravi commented Feb 9, 2015

@AVGP Unfortunately I might have misunderstood your commit... I had hoped for more context for the error message, but it seems to me your change does not provide that.

@ravi
Copy link
Copy Markdown

ravi commented Feb 9, 2015

For my ~ 600 line JSON5 config file, the below is mostly useless as an error report, I am afraid:

/tmp/node_modules/json5/lib/json5.js:54
            throw error;
                  ^
SyntaxError: Expected ']' instead of '{'

aseemk
aseemk reviewed Feb 9, 2015
View reviewed changes
Comment thread lib/cli.js
Copy link
Copy Markdown
Member

@aseemk aseemk Feb 9, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of referencing args[2] in multiple spots, let's add a variable to save it, e.g. cmd after args.

Copy link
Copy Markdown
Member

@aseemk aseemk Feb 9, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, is this boolean logic right? I thought && takes precedence over ||, so you might need to wrap the -c || --validate check in parentheses.

@aseemk
Copy link
Copy Markdown
Member

aseemk commented Feb 9, 2015

Nice work otherwise @AVGP. Thank you for the pull request! =)

@aseemk
Copy link
Copy Markdown
Member

aseemk commented Feb 9, 2015

Good point @ravi. So this won't fix that. Please feel free to open a new issue to add line and column number to syntax error messages. Hopefully it won't be too hard for someone to take a stab at.

@ravi
Copy link
Copy Markdown

ravi commented Feb 9, 2015

Created #86 . Thanks.

@jordanbtucker jordanbtucker mentioned this pull request Mar 17, 2016
Closed
@jordanbtucker jordanbtucker added this to the v1.0.0 milestone Sep 20, 2017
@jordanbtucker jordanbtucker self-assigned this Sep 20, 2017
@jordanbtucker
Copy link
Copy Markdown
Member

jordanbtucker commented Sep 21, 2017

Fixed in 35269da

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@jordanbtucker jordanbtucker

Labels

enhancement 👍

Projects

None yet

Milestone

v1.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@AVGP @ravi @aseemk @jordanbtucker