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

Adding a --validate CLI option #84

Closed
wants to merge 1 commit into from
Closed

Conversation

AVGP
Copy link

@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

ravi commented Feb 9, 2015

Can we have this? (please!)

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

aseemk commented Feb 9, 2015

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

@ravi
Copy link

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

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 '{'

var args = process.argv;

if (args.length < 4 || args[2] !== '-c') {
if (args[2] === '-c' || args[2] === '--validate' && args.length < 4) {
Copy link
Member

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
Member

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
Member

aseemk commented Feb 9, 2015

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

@aseemk
Copy link
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

ravi commented Feb 9, 2015

Created #86 . Thanks.

@jordanbtucker
Copy link
Member

Fixed in 35269da

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

Successfully merging this pull request may close these issues.

None yet

4 participants