feat: add problem matcher for VNU validator - Githubissues

w3c / spec-prod

GitHub Action to build ReSpec/Bikeshed specs, validate output and publish to GitHub pages or W3C

https://w3c.github.io/spec-prod/

MIT License

34 stars 21 forks source link

feat: add problem matcher for VNU validator #120

Closed nschonni closed 2 years ago

nschonni commented 2 years ago

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

sidvishnoi commented 2 years ago

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 commented 2 years ago

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 https://github.com/actions/toolkit/issues/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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 https://github.com/w3c/spec-prod/pull/120#issuecomment-1004268114 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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

https://github.com/w3c/screen-wake-lock is processed by ReSpec (it's not raw HTML), so the output line numbers that vnu will get will be different than ones shown in index.html.
https://github.com/w3c/gyroscope that uses Bikeshed into another branch: it actually runs only in same main branch.

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

Builds file using ReSpec/Bikeshed.
Runs various validators (including vnu) on the output of above step.
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 commented 2 years ago

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

sidvishnoi commented 2 years ago

Filed https://github.com/w3c/spec-prod/issues/122/