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 some PR automation once we start 1.3 #1106

Open
jnurthen opened this issue Oct 25, 2019 · 25 comments
Open

Add some PR automation once we start 1.3 #1106

jnurthen opened this issue Oct 25, 2019 · 25 comments
Assignees
Labels
editorial a change to an example, note, spelling, grammar, or is related to publishing or the repo good first issue process
Projects
Milestone

Comments

@jnurthen
Copy link
Member

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
Copy link
Contributor

joanmarie commented Oct 25, 2019

  • 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. 😃

@nschonni
Copy link
Contributor

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 jnurthen added this to the ARIA 1.3 milestone Oct 31, 2019
@jnurthen
Copy link
Member Author

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

@nschonni
Copy link
Contributor

nschonni commented Nov 15, 2019

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
Copy link
Contributor

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

@jnurthen
Copy link
Member Author

@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
Copy link
Contributor

carmacleod commented Nov 15, 2019

@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
Copy link
Member Author

@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
Copy link
Contributor

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

@charmarkk charmarkk assigned charmarkk and unassigned charmarkk Feb 13, 2020
@schne324
Copy link
Contributor

schne324 commented Feb 13, 2020

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

@schne324 schne324 self-assigned this Feb 13, 2020
@sinabahram
Copy link

sinabahram commented Feb 13, 2020 via email

@marcoscaceres
Copy link
Member

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
Copy link

sinabahram commented Feb 13, 2020 via email

@marcoscaceres
Copy link
Member

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
Copy link

sinabahram commented Feb 14, 2020 via email

@marcoscaceres
Copy link
Member

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
Copy link

sinabahram commented Feb 14, 2020 via email

@marcoscaceres
Copy link
Member

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: w3c/payment-request#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
Copy link
Contributor

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
Copy link

sinabahram commented Feb 14, 2020 via email

@sinabahram
Copy link

sinabahram commented Feb 14, 2020 via email

@schne324
Copy link
Contributor

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

@schne324
Copy link
Contributor

schne324 commented Oct 22, 2020

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: schne324/github-actions-test#2

@jnurthen
Copy link
Member Author

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

@schne324
Copy link
Contributor

schne324 commented Nov 19, 2020

@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

@pkra pkra mentioned this issue Nov 5, 2021
@spectranaut spectranaut added this to Good first issues in ARIA 1.3 Jun 7, 2022
@spectranaut spectranaut added the editorial a change to an example, note, spelling, grammar, or is related to publishing or the repo label Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial a change to an example, note, spelling, grammar, or is related to publishing or the repo good first issue process
Projects
ARIA 1.3
Good first issues
Development

No branches or pull requests

9 participants