-
-
Notifications
You must be signed in to change notification settings - Fork 79k
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
Add back vnu-jar for HTML validation #24149
Conversation
4e505a9
to
69b18cc
Compare
206ee5f
to
6532cb0
Compare
I believe this is ready. Perhaps we'd like to wait until the jekyll-toc errors are fixed upstream before merging this... or not. @patrickhlauke: which of the other errors should be ignored? Note that I'm ignoring warnings on purpose. We might want to revisit this later. |
@XhmikosR I think you should add |
I think I'll wait a couple of days since the jekyll-toc issue seems to be WIP. We still need @patrickhlauke's feedback about the remaining errors and which we should ignore. |
cc696b7
to
e619312
Compare
cd93f30
to
26db653
Compare
Does anybody have any idea why ignoring specific errors doesn't work? EDIT: All right, after discussing this with @sideshowbarker on IRC, it appears the messages are inconsistent thus leading to this issue. I'll wait a bit for this to be addressed and we should be ready to merge in a few days, hopefully. |
fd3c19b
to
66cce53
Compare
@bardiharborow @Johann-S @mdo: I changed this PR to not remove htmlhint and just add vnu-jar which will run if Java is available. What do you think? There's also the need to verify what we ignored in this branch. @patrickhlauke: can you have a quick look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 just a question, so we will use your script only if folks have Java ? Not on Travis ?
|
||
childProcess.exec('java -version', function (error) { | ||
if (error) { | ||
console.error('Skipping HTML lint test; Java is missing.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you return directly after that console.error you should throw an error instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to fail here; if Java isn't installed we are just skipping vnu-jar.
@XhmikosR seems fine to me.
I don't have an issue with merging now and then removing some ignores later. |
@Johann-S: Travis has Java so it will run there. See also this PR's Travis builds :) |
I'm gonna merge this but I still think we need to go through our ignores. For example, the error about title on circle seems legit to me and it's the issue I was telling @Johann-S the other day on Slack; this is non standard. |
After discussing this with @bardiharborow, I think this is the best middle solution; if Java is available HTML validation will run, otherwise it will be skipped.
Also, this is a lot faster than the html-validator solution and we don't introduce another point of external failure (like with Sauce Labs).
TODO:
Fixes #23171