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

Add vnu.jar support #2469

Merged
merged 1 commit into from
Oct 2, 2019
Merged

Add vnu.jar support #2469

merged 1 commit into from
Oct 2, 2019

Conversation

XhmikosR
Copy link
Contributor

@XhmikosR XhmikosR commented Aug 26, 2019

This is WIP because there are some things that need to be addressed:

  1. anchor-markdown-headings.js: fix IDs to be unique. #2441 needs to be merged
  2. Convert a few posts to Markdown #2437 probably needs to be merged too
  3. There's a bug in vnu.jar, I have already discussed with sideshowbarker (not CC'ing right now), that causes the build to exit with non-zero.
  4. I'd prefer it if we could actually fix the table errors, because these are real issues:
    "file:/home/travis/build/XhmikosR/nodejs.org/build/en/download/index.html":276.50-277.32: error: Table column 3 established by element "td" has no cells beginning in it.
    "file:/home/travis/build/XhmikosR/nodejs.org/build/en/download/index.html":277.113-278.32: error: Table column 7 established by element "td" has no cells beginning in it.
    
  5. The license of the new file might need to be adapted for this repo. It's just a simple script so it's OK to change it, especially since I also adapted it to fit our needs

Note that this is how I noticed and fixed many of the issues so far, like bae664a, b533fcd, a714afc, ceee600 etc.

IMO this should definitely land as soon as we sort out the above points. Personally I'd remove htmllint too; I'm having issues with it on Windows and it just doesn't seem to catch any real issues.

Marking it as draft for further discussion and tweaks.

vnu.jar: https://github.com/validator/validator

Closes #2398

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Sep 4, 2019

I think we should try fixing the table errors. I'm gonna revert the ignore so that we see the errors, and someone might be able to help fix those. I believe the wrong exit code comes from this for some reason.

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Sep 5, 2019

Alright, I don't know how to fix the table errors, so I have them ignored for now.

This is ready for review. Note that on Windows there will be a failure due to #2422. This isn't triggered on nix, but I've yet to figure out the root cause (like I never managed to figure it out for the slashes, hence the workaround we have).

@XhmikosR XhmikosR marked this pull request as ready for review September 6, 2019 06:26
@XhmikosR
Copy link
Contributor Author

XhmikosR commented Sep 6, 2019

After #2511 CI will be green. Generally that is why I insist that branches need to be up to date and we have the button.

@XhmikosR
Copy link
Contributor Author

Can we move with this?

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Oct 1, 2019

Can I get some reviews please?

This is the only proper way to make sure we are valid and we don't have issues like duplicate IDs and so on. Note that I intentionally skip the test if Java isn't available, and that I haven't added the script in npm t like I haven't done it with linkinator, because these tests are a little slow.

On CI Java is present so this will run there.

If Java isn't present, the test will be skipped. On CI Java is available, though.
@Trott
Copy link
Member

Trott commented Oct 1, 2019

@nodejs/website

Copy link
Member

@phillipj phillipj left a comment

Choose a reason for hiding this comment

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

LGTM

@Trott Trott merged commit db76a4f into nodejs:master Oct 2, 2019
@XhmikosR XhmikosR deleted the master-xmr-vnu branch October 2, 2019 16:02
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.

Fix HTML validation errors
3 participants