Add some PR automation once we start 1.3

[jnurthen]

Some ideas....

• Making sure spacing is consistent
• Spelling
• Alexjs

others?

@nschonni - you have done a lot of this for practices. Is there anything we could use in the spec.

[joanmarie]

• No trailing whitespace
• HTML validation
• No whitespace-only changes

In addition, I would like lines to not be paragraphs long. It makes diffs* super hard to read and find what actually changed. Thus I would like to:

1. Agree on the max line length, and having done so
2. Use automation to enforce it

*Edit: By "diffs" I'm talking about actual diffs (e.g. when you run git diff from the command line). Yes, I know that we have the pull request preview and diff tools. I do not care. :smiley:

[nschonni]

Some of the whitespace things could be done with https://github.com/jedmao/eclint but it's not that language aware. EX: it will flag 3 space indents when configured for 2 spaces, but can't tell if a 2 vs. 4 space indent is correct for an HTML file indenting on a particular line. It should cover the line length part though.

* Alexjs

There are also "write good" plugins for some linters like remark-lint or erata-ai/Vale

[jnurthen]

Can we just use prettier for the formatting? Any thoughts?

[nschonni]

I haven't really used prettier on anything myself. Every time I looked at it I hit issues because it is intentionally opinionated and less configurable than other libraries. If it works for other people though, go for it!

[carmacleod]

ARIA and APG should try to coordinate tooling if possible. There has been some discussion about this for APG recently: https://github.com/w3c/aria-practices/issues/1180

[jnurthen]

@carmacleod while I agree in principle there are differences. APG needs tooling to support JavaScript, CSS and HTML - ARIA only requires HTML. I don't want to add complexity to ARIA tooling if it is only needed to support CSS and JS.

[carmacleod]

@jnurthen Good point. How about "wherever it makes sense to do so, ARIA and APG should coordinate tooling". ;) There was discussion specifically about prettier in the APG issue.

@joanmarie RE: max line length

Just FYI, the WHATWG HTML spec contributing guidelines say to use a column width of 100 characters.

Personally, I find that a bit short for a typical line (I find it hard to read when it's broken up like that), and would prefer somewhere closer to 200 (since my editor can display 200 characters at a decent enough size on my monitor without scrolling 😄 ). However I would defer to group consensus. :)

[jnurthen]

@carmacleod this is the beauty of something like prettier..... You can override your line length to whatever you like. A pre-commit script can be set to run and prettier then makes the commit the right line length in the repository.

I'm with WHATWG on this though. 100 is plenty. 200 is way too long.

[joanmarie]

I use a rotated monitor. 130 chars is the max that fit for me. So 100-120 seems sane.

[schne324]

A pre-commit script can be set to run and prettier then makes the commit the right line length in the repository.

☝️ this is really easy to implement but it would require that all contributors have node/npm installed...is that okay? If not, I could try to write a github prettier action

[sinabahram]

Question, is there not a GitHub workflow that would facilitate this without requiring it to be run pre-commit? I understand it would necessitate an automatic commit to make sure diffing doesn’t get affected, but that can be the fallback at least, no?

Idea:

Someone commits a change

GitHub runs a “thing”, where thing is to be defined, to do the prettification and then commits the changes

Diffs can now be consistent, always being run against the post-pretty output

Assumption:

Running this script on or outside of GitHub on something that has already been processed by it results in no changes, of course.

[marcoscaceres]

Hi All, I suggest using HTML5 tidy:

• fixes all the whitespace,
• checks/fixes the markup.

And for validation, the respec-validator:

• checks against W3C Validator
• checks there are no broken links (internal and external!)
• Checks there are no ReSpec errors in your document.
• Easy to run via a Github action (we use this for other specs).

[sinabahram]

Am I correct that the implication would be that if someone wishes to provide a PR, they would need to run this toolchain locally first since their commits would not go to this repository where the action resides? Also, follow-up would be could they simply set up that action on their own repository thus obviating that?

[marcoscaceres]

Am I correct that the implication would be that if someone wishes to provide a PR, they would need to run this toolchain locally first since their commits would not go to this repository where the action resides?

No 🎉 we would run it on the server... but people would have the choice to run it locally (see below).

Also, follow-up would be could they simply set up that action on their own repository thus obviating that?

Maybe... but that's complicated....

For the HTML5 tidy integration, we need someone to create the GitHub action for us. Ideally, tidy would run when a pull request is merged. The workflow would be:

1. editor makes spec changes locally. Runs HTML tidy locally. Sends Pull Request.
2. people on GitHub make suggestions, which will change the formatting.
3. When the pull request is merged, the "tidy" action runs.

We might be able to hire someone to do that for us via ReSpec's fund if people are interested (if a few of us throw in a few bucks, maybe we probably hire someone quickly to do it). Otherwise, we can see if the W3C Staff can help us figure out how to run tidy as a GitHub action.

I think a lot of working groups would benefit from this.

[sinabahram]

Hmm, just to make sure I understand, I know you said they wouldn’t need to run it because the server would, but are you saying that the server would run that action on a proposed/unaccepted PR? Because that’s when the diffs would need to happen, to allow people to then accept the PR.

I ask because in your example, you have the editor running this locally, which gets to the heart of my question. Why are they running it locally if the above is true?

[marcoscaceres]

I know you said they wouldn’t need to run it because the server would, but are you saying that the server would run that action on a proposed/unaccepted PR?

Correct. Only on the accepted PR.

Why are they running it locally if the above is true?

It's polite to the reviewers, and it makes it easier to see the diffs.

But it's not a prerequisite.

This caters for new contributors, or those who might struggled setting up command line tools. It also allows folks to make "drive-by" edits directly on GitHub. But it assured that the final merged spec is always nice and tidy :)

[sinabahram]

Ok, so this is where my confusion comes from. You agreed with me, but then stated the opposite of what I said.

I said:

“I know you said they wouldn’t need to run it because the server would, but are you saying that the server would run that action on a proposed/unaccepted PR?”

Your response:

“Correct. Only on the accepted PR.”

See my confusion?

Apologies in advance for belaboring the point. I promise I’m not trying to be difficult. Just want to make sure I fully understand the implications. You actually hit upon my concern with your subsequent comment about new folks offering edits.

[marcoscaceres]

Apologies in advance for belaboring the point. I promise I’m not trying to be difficult. Just want to make sure I fully understand the implications. You actually hit upon my concern with your subsequent comment about new folks offering edits.

No problem at all. Sorry for not being clear. You are asking all the right questions.

This solution caters for all cases: the starting point for any contributor will always be a tidy spec.

For larger edits, it may be prudent to ask the contributor to run tidy locally sometimes - but this is super rare in my experience (and if those cases, it's easy for a more experienced editor to just help out and run tidy for contributor if they need help, and paste in the result back into the PR).

It might help to see a real example of exactly this process: https://github.com/w3c/payment-request/pull/873

We've collaborated on pretty massive PR, which got very messy, so we've been tidying it as we go (search for "tidy" and you will see both the discussion, and me occasionally re-committing a tidy'ed version).

[nschonni]

Maybe an example of what is being used in the aria-practices repo today. For editors working locally, Husky is used to run a bunch of linting that prevents a local commit till the author addresses the linting issues. It also uses --fix parameters to update the files when they can be automatically fixed https://github.com/w3c/aria-practices/blob/a2a9d6ddda99902b1a63b6e96c8c8fed279ab0a9/package.json#L45-L59

When the PR is made, Travis-CI runs similar linting checks, along with longer running tests that would be to "heavy" to run on every local commit like the integration tests https://github.com/w3c/aria-practices/blob/master/.travis.yml#L19

The Travis-CI setup also supports catching issues cause by editing directly on GitHub, but can't offer the same automatic cleanup that the local hooks can.

[sinabahram]

I see, ok I was missing that human piece to this RE submitting a tidied version for someone. That clarifies things.

Thanks

[sinabahram]

This was brought to my attention as an action that does html validation, but I haven’t delved into it at all yet. Possible that it could be helpful as a base.

https://github.com/marketplace/actions/html5-validator

[schne324]

I've used https://github.com/marketplace/actions/prettier-action on a project and it seems to work quite well.

[schne324]

I've set up a test repo for running prettier on html files in a pull request. Feel free to poke around and even create a PR to see it in action

I committed the following weirdly formatted html with tons of whitespace/indentation weirdness:

<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <meta name="viewport" content="width=device-width, initial-scale=1.0">
  <title>Foo</title>
</head>
<body>
  <h1>

         TONS OF WHITESPACE IN THE HEADING
  </h1>
                                            <p><strong><span>Hi</span></strong></p>
                    <h2>Foo

</h2>
</body>
</html>

and the github action automagically formatted/cleaned it to:

<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="UTF-8" />
    <meta name="viewport" content="width=device-width, initial-scale=1.0" />
    <title>Foo</title>
  </head>
  <body>
    <h1>TONS OF WHITESPACE IN THE HEADING</h1>
    <p>
      <strong><span>Hi</span></strong>
    </p>
    <h2>Foo</h2>
  </body>
</html>

(view pr from above example)

Any thoughts / feedback welcome!

p.s. regarding @joanmarie 's comment about max line length, I set up a PR which shows max lengths being enforced: https://github.com/schne324/github-actions-test/pull/2

[jnurthen]

@schne324 finally got round to looking at this and https://github.com/dequelabs/guided-tool-test-pages/ gives a 404

[schne324]

@jnurthen it appears I linked to the wrong repo 🤦 . I've updated the link to the originally intended repo: https://github.com/schne324/github-actions-test where there are a couple example prs