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

feat: add problem matcher for VNU validator #120

Closed
wants to merge 1 commit into from

Conversation

nschonni
Copy link
Contributor

@nschonni nschonni commented Jan 2, 2022

Copied over from aria-practices where it's already used.
Not sure if there is a better location to place the JSON file though

Comment on lines 8 to 10
"file": 1,
"line": 2,
"column": 3,
Copy link
Member

Choose a reason for hiding this comment

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

As validation is run on generated HTML files, line numbers and file name will end up marking wrong positions. I think we can fix the filename part by renaming the file to have same name as source (for validation only; revert it once validation step is done), but we should skip the line/column numbers as they'll be misleading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think running it on the original file will just give the wrong lines, since there are all the boilerplate from Bikeshed/respec that would make it not line up.
I guess this current version would mostly help those that do have the generated files in the source, but wouldn't change anything for those that don't

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I wasn't clear. I mean, we should rename the built file to the one in repo (source file) so GitHub can show annotations on the right file, even if the line numbers are wrong.

action.yml Outdated Show resolved Hide resolved
__dirname,
"validate-markup-problem-matcher.json",
);
core.info(`##[add-matcher]${matchersPath}`);
Copy link
Member

Choose a reason for hiding this comment

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

I was hoping there to be a core.addMatcher(filePath), but it doesn't exist. Let's keep it in action.yml then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at those other setup-* Actions, I think you were right around the path relative to the action, so this might be better (and more cross platform)

@sidvishnoi
Copy link
Member

Also, I wonder if we should use problem matchers or parse the stdout/stderr of the shell command and then core.warn (this will give us more control than the matcher, which is limited to regex).

@nschonni
Copy link
Contributor Author

nschonni commented Jan 3, 2022

Also, I wonder if we should use problem matchers or parse the stdout/stderr of the shell command and then core.warn (this will give us more control than the matcher, which is limited to regex).

From actions/toolkit#186 it looks like there may not be an easy way to do this right now. But from there, it does look like there is a similar "echo" approach that can do it

@sidvishnoi
Copy link
Member

sidvishnoi commented Jan 3, 2022

We can use core.warning(msg, { file, title, startLine }) (similarly for core.error and core.notice).
See https://github.com/actions/toolkit/blob/daf8bb00606d37ee2431d9b1596b88513dcf9c59/packages/core/src/core.ts#L245.

@nschonni
Copy link
Contributor Author

nschonni commented Jan 3, 2022

I think i've taken this one as far as I can. If you've got a different approach you'd like, feel free to close

@sidvishnoi
Copy link
Member

Sorry if this exercise felt annoying. My concern with the matcher approach is that it'll end up adding the annotations to wrong file (a file that doesn't exist in repo) unless we rename the file before running validator. With the JS based approach, we can use a different filename as I mentioned in #120 (comment) and use lineNumber = 1 so the annotations are added to top of source file (and not at wrong place as per the generated output file).

@nschonni
Copy link
Contributor Author

nschonni commented Jan 3, 2022

If there is no matching file, GitHub doesn't add annotations to the UI, so it won't change anything for those that don't have the generated files in the source

@sidvishnoi
Copy link
Member

Ah! I didn't know that!

But then what benefit does it add, as:

  • if the file doesn't exist in source, it won't show the annotations.
  • if the file exists, the line numbers will be wrong (line numbers based on generated HTML file but shown marked on source file)

@nschonni
Copy link
Contributor Author

nschonni commented Jan 3, 2022

EX: https://github.com/w3c/screen-wake-lock it's running on the HTML file in the source, so the line numbers will be correct and work.
Something like https://github.com/w3c/gyroscope that uses Bikeshed into another branch, just won't show file level annotations

@sidvishnoi
Copy link
Member

Sorry, but the examples you've shared both run the validator on generated output files.


Just to be clear, here's what spec-prod does:

  1. Builds file using ReSpec/Bikeshed.
  2. Runs various validators (including vnu) on the output of above step.
  3. Deploys output of step 1 to GitHub Pages (to a separate branch in same repo) and/or W3C.

Step 2 above is the reason why line numbers will always be wrong, unless there's some sort of sourcemap generation by ReSpec/Bikeshed that vnu can use.

@sidvishnoi
Copy link
Member

I'm inclined to close this PR for now, but I'll open an issue for this feature.

@nschonni nschonni closed this Jan 3, 2022
@nschonni nschonni deleted the vnu-problem-matcher branch January 3, 2022 18:55
@sidvishnoi
Copy link
Member

sidvishnoi commented Jan 3, 2022

Filed #122

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.

2 participants