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

give a non-zero exit code when validation fails #4

Merged
merged 1 commit into from Sep 21, 2016

Conversation

afeld
Copy link
Contributor

@afeld afeld commented Sep 21, 2016

Fixes #1.

Previously, the CLI would only fail in cases where the file/URL couldn't be found. Now, it will fail in those cases, as well as if the markup is found to be invalid. This is the behavior I (and I think others?) would expect, but curious to hear thoughts. Thanks!

/cc @mikebell @radek-holy

}
} else {
msg = data
if (data.includes("There were errors")) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is brittle, but I couldn't figure out a cleaner way of handling it without also making changes to html-validator.

}

if (validationFailed) {
console.error(msg)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Undecided about whether these errors should print to STDOUT or STDERR.

Copy link
Owner

Choose a reason for hiding this comment

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

My instincts says STDOUT. It's not an error message IMO. I'll change it when I update.

Previously, the CLI would only fail in cases where the file/URL couldn't
be found. Now, it will fail in those cases, as well as if the markup is
found to be invalid.
@afeld
Copy link
Contributor Author

afeld commented Sep 21, 2016

Also, the test failures are legitimate...if you're happy with this approach, not sure what to do about them.

@zrrrzzt
Copy link
Owner

zrrrzzt commented Sep 21, 2016

Hi there @afeld

Thank you for your interest in this module. I almost got a bit scared when I realised how old the code is.

Regarding a non-zero exit if validation fails. I see the value of a change like that. I'll merge your PR now, but I'd like to do a full updates before I push a new release to npm. No promises, but probably later today or tomorrow

@zrrrzzt zrrrzzt merged commit 6dd1f69 into zrrrzzt:master Sep 21, 2016
@zrrrzzt
Copy link
Owner

zrrrzzt commented Sep 21, 2016

A quick question @afeld : I can't find any suitable exit codes for failed validation.
2 is actually reserved https://github.com/nodejs/node-v0.x-archive/blob/master/doc/api/process.markdown#exit-codes and the others are not quite right.

What I could do as an alternative solution is to send a 'Page is not valid' message to stderr before I send the complete result to stdout. Would that solve your problem or does it have to be an exitCode?

@afeld
Copy link
Contributor Author

afeld commented Sep 21, 2016

I can't find any suitable exit codes for failed validation

I don't personally care which exit code is used...just figured it might be useful to someone else to distinguish the type of error based on the exit code, and it was easy to add. Feel free to change it to process.exit(1).

2 is actually reserved

Today I learned!

@zrrrzzt
Copy link
Owner

zrrrzzt commented Sep 23, 2016

New version published. Exits with 1 if validation fails. Happy @afeld ?

@afeld
Copy link
Contributor Author

afeld commented Sep 27, 2016

Works great, thanks!

afeld added a commit to startup-systems/static that referenced this pull request Sep 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants