FR: Establish a default python formatter / linter and enforce it via CI

[thiagowfx]

Note: Originally copied / imported from https://github.com/web-platform-tests/wpt/issues/42710. We'll keep the other bug open to track the implementation side of this FR.

So long as we do not agree on a default formatter for this repository, back-end-forth diff noise and bikeshedding keeps coming up (e.g.).

It would be the best to everyone, and for improved devex, to agree to use one and only one formatter. Additionally, to ensure it sticks, we should add some CI job to enforce that no violations are introduced.

The status quo (at least within the realm of WebDriver BiDi) seems to be the following:

• Google / Chromium and the Chromium BiDi Mapper use yapf v0.31.0.
• Mozilla / Firefox uses black.

The following AIs should happen:

1. [ ] agree on which formatter to use: yapf, black, or something else?
2. [ ] globally format the codebase using it
3. [ ] add a (non-optional?) CI check to enforce it

Resources:

• yapf: https://github.com/google/yapf
• black: https://github.com/psf/black

cc @juliandescottes @past

[jgraham]

I will note that in the past I have been very skeptical about making this work across multiple repos.

The problem is that each repo needs to be using exactly the same version of exactly the same formatter. Otherwise, if there are any auto-formatting differences, two way sync becomes ~impossible because a changeset that can land in one place will fail CI in the other and vice versa.

One could try to get around this by just running the relevant formatter on all inbound changes, effectively allowing each repo to enforce its own formatting preferences, but this has significant costs, not just in that it requires changes to all the sync bots, but also that line-based diffs between the repos no longer represent real changes.

So in theory one would have to pick a specific revision of a specific formatter for wpt, and provide the tooling to run it. But the deverloper experience of that formatter being different from the vendor repo would be terrible; afaik most auto-formatting setups don't easily support using a totally different tool depending on the directory, whereas they generally do allow excluding directories from auto formatting.

[juliandescottes]

My initial comment was more about having a common formatter between folks contributing to bidi tests, as we are bound to edit and format the same files regularly. I also think that enforcing something at the repo level would be very challenging.

Edit: And since autoformatting was mentioned, I would note, that I'm completely ok with running a manual formatter for wdspec changes on the side, if that avoids pollution in diffs. But I might be in the minority here.

[thiagowfx]

cc @whimboo

[whimboo]

Note that we have already linting setup for files like /tools/webdriver but not for wdspec tests. So maybe we can just expand the current linter setup to also cover /webdriver/?

[OrKoN]

Let's try to expand it? So far I noticed that formatting discussions take a significant portion of the review. At least, it would be great if there was a shared linter configuration that one could run locally and be certain that most of the formatting quirks align with conventions

[jgraham]

The linter should be safe enough, I think, because it's generally possible to find some code format that satisfies multiple versions of a linter, unlike autoformatters which generally define a single correct representation of the given code.

[thiagowfx]

Note that we have already linting setup for files like /tools/webdriver but not for wdspec tests.

Where did you see this? I grep'ed for it in the wpt/ repo but could not find it.

[whimboo]

Note that we have already linting setup for files like /tools/webdriver but not for wdspec tests.

Where did you see this? I grep'ed for it in the wpt/ repo but could not find it.

Try to push wrongly formatted code via a PR and you will see that the unit tests are failing. That is only happening for the webdriver client code but not the webdriver tests. I'm not exactly sure where the definitions actually are. @jgraham will know more here.

[gsnedders]

See also #129, which is about prettier (for HTML/CSS/JS/etc).

The current PR just adds a bunch of extra lints; I don't think we have any precedent for whether lints require an RFC?

[thiagowfx]

The current PR just adds a bunch of extra lints; I don't think we have any precedent for whether lints require an RFC?

Indeed, linting is just a drive-by change, and I linked back to this just for completeness.

For the core of this PR we first need to agree upon a formatter...should we create a poll to decide which one, maybe?

[jgraham]

Unless we have a way to address the concerns in https://github.com/web-platform-tests/rfcs/issues/171#issuecomment-1777545307 we can't adopt a formatter.

[thiagowfx]

Unless we have a way to address the concerns in https://github.com/web-platform-tests/rfcs/issues/171#issuecomment-1777545307 we can't adopt a formatter.

With yapf this is definitely an issue. Different versions yield different results.

We could perhaps test multiple versions of black. It's supposed to be non-opinionated and stable.

[jgraham]

https://phabricator.services.mozilla.com/D186092 is gecko updating black a few months ago. It reformatted 290 files. I think that's sufficient to prove that we'd need a system that required every user to have the precise same version of the code formatter.

[thiagowfx]

OK, thanks for the black anecdote. Then I do not see any other way to move this forward. Unless e.g. we moved to pipenv or something similar to have dependency pinning on python tooling.

[Ms2ger]

We can pin the dependencies; the issue is that all repositories that embed wpt (e.g. Chrome, Firefox, Servo) would need to pin to the same version and updates would need to be coordinated across all of them

[foolip]

It would be a lot of work, but a path forward could be managing dependencies in WPT and downstream repos in a way that downstreams "simply" use the Pipenv file (or something) and there is no manual work needed. So WPT import tooling would need to automatically sync dependencies as well.

I think Chromium is pretty far from this being easy, however.

[thiagowfx]

Carrying over @gsnedders's comment (https://github.com/web-platform-tests/wpt/pull/43096#discussion_r1395821904): How about linters? How could we enforce e.g. flake8?