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

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

Open
3 tasks
thiagowfx opened this issue Oct 24, 2023 · 17 comments
Open
3 tasks

Comments

@thiagowfx
Copy link
Member

thiagowfx commented Oct 24, 2023

Note: Originally copied / imported from web-platform-tests/wpt#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:

cc @juliandescottes @past

@jgraham
Copy link
Contributor

jgraham commented Oct 24, 2023

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

juliandescottes commented Oct 24, 2023

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

cc @whimboo

@whimboo
Copy link
Contributor

whimboo commented Nov 8, 2023

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

OrKoN commented Nov 10, 2023

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

jgraham commented Nov 10, 2023

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

thiagowfx commented Nov 10, 2023

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

whimboo commented Nov 13, 2023

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.

thiagowfx pushed a commit to web-platform-tests/wpt that referenced this issue Nov 13, 2023
thiagowfx pushed a commit to web-platform-tests/wpt that referenced this issue Nov 14, 2023
thiagowfx pushed a commit to web-platform-tests/wpt that referenced this issue Nov 14, 2023
thiagowfx pushed a commit to web-platform-tests/wpt that referenced this issue Nov 16, 2023
@gsnedders
Copy link
Member

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

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

jgraham commented Nov 16, 2023

Unless we have a way to address the concerns in #171 (comment) we can't adopt a formatter.

@thiagowfx
Copy link
Member Author

thiagowfx commented Nov 16, 2023

Unless we have a way to address the concerns in #171 (comment) 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
Copy link
Contributor

jgraham commented Nov 16, 2023

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

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

Ms2ger commented Nov 16, 2023

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

foolip commented Nov 16, 2023

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 pushed a commit to web-platform-tests/wpt that referenced this issue Nov 17, 2023
thiagowfx pushed a commit to web-platform-tests/wpt that referenced this issue Nov 17, 2023
Address flake8 findings in webdriver/
Rename duplicate test to test_params_pointer_action_move_coordinate_missing

Bug: web-platform-tests/rfcs#171
Cmdline: flake8 --append-config=tools/flake8.ini webdriver/tests/bidi                                                                                                                                                 :
thiagowfx pushed a commit to web-platform-tests/wpt that referenced this issue Nov 17, 2023
Rename duplicate test to test_params_pointer_action_move_coordinate_missing

Bug: web-platform-tests/rfcs#171
Cmdline: flake8 --append-config=tools/flake8.ini webdriver/tests/bidi                                                                                                                                                 :
@thiagowfx
Copy link
Member Author

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

@thiagowfx thiagowfx changed the title FR: Establish a default python formatter and enforce it via CI FR: Establish a default python formatter / linter and enforce it via CI Nov 17, 2023
gsnedders pushed a commit to web-platform-tests/wpt that referenced this issue Nov 17, 2023
Rename duplicate test to test_params_pointer_action_move_coordinate_missing

Bug: web-platform-tests/rfcs#171
Cmdline: flake8 --append-config=tools/flake8.ini webdriver/tests/bidi
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Nov 22, 2023
…ver/, a=testonly

Automatic update from web-platform-tests
infra: address flake8 findings in webdriver/

Rename duplicate test to test_params_pointer_action_move_coordinate_missing

Bug: web-platform-tests/rfcs#171
Cmdline: flake8 --append-config=tools/flake8.ini webdriver/tests/bidi
--

wpt-commits: 605a1999160c97f971fce384dac5cddd4ca0b2d1
wpt-pr: 43096
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Nov 22, 2023
…ver/, a=testonly

Automatic update from web-platform-tests
infra: address flake8 findings in webdriver/

Rename duplicate test to test_params_pointer_action_move_coordinate_missing

Bug: web-platform-tests/rfcs#171
Cmdline: flake8 --append-config=tools/flake8.ini webdriver/tests/bidi
--

wpt-commits: 605a1999160c97f971fce384dac5cddd4ca0b2d1
wpt-pr: 43096
vinnydiehl pushed a commit to vinnydiehl/mozilla-unified that referenced this issue Nov 24, 2023
…ver/, a=testonly

Automatic update from web-platform-tests
infra: address flake8 findings in webdriver/

Rename duplicate test to test_params_pointer_action_move_coordinate_missing

Bug: web-platform-tests/rfcs#171
Cmdline: flake8 --append-config=tools/flake8.ini webdriver/tests/bidi
--

wpt-commits: 605a1999160c97f971fce384dac5cddd4ca0b2d1
wpt-pr: 43096
vinnydiehl pushed a commit to vinnydiehl/mozilla-unified that referenced this issue Nov 24, 2023
…ver/, a=testonly

Automatic update from web-platform-tests
infra: address flake8 findings in webdriver/

Rename duplicate test to test_params_pointer_action_move_coordinate_missing

Bug: web-platform-tests/rfcs#171
Cmdline: flake8 --append-config=tools/flake8.ini webdriver/tests/bidi
--

wpt-commits: 605a1999160c97f971fce384dac5cddd4ca0b2d1
wpt-pr: 43096
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Nov 30, 2023
…ver/, a=testonly

Automatic update from web-platform-tests
infra: address flake8 findings in webdriver/

Rename duplicate test to test_params_pointer_action_move_coordinate_missing

Bug: web-platform-tests/rfcs#171
Cmdline: flake8 --append-config=tools/flake8.ini webdriver/tests/bidi
--

wpt-commits: 605a1999160c97f971fce384dac5cddd4ca0b2d1
wpt-pr: 43096

UltraBlame original commit: 98ddfdc5d64d44b5c70b46671958a3cd76fa988a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Nov 30, 2023
…ver/, a=testonly

Automatic update from web-platform-tests
infra: address flake8 findings in webdriver/

Rename duplicate test to test_params_pointer_action_move_coordinate_missing

Bug: web-platform-tests/rfcs#171
Cmdline: flake8 --append-config=tools/flake8.ini webdriver/tests/bidi
--

wpt-commits: 605a1999160c97f971fce384dac5cddd4ca0b2d1
wpt-pr: 43096

UltraBlame original commit: 98ddfdc5d64d44b5c70b46671958a3cd76fa988a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Nov 30, 2023
…ver/, a=testonly

Automatic update from web-platform-tests
infra: address flake8 findings in webdriver/

Rename duplicate test to test_params_pointer_action_move_coordinate_missing

Bug: web-platform-tests/rfcs#171
Cmdline: flake8 --append-config=tools/flake8.ini webdriver/tests/bidi
--

wpt-commits: 605a1999160c97f971fce384dac5cddd4ca0b2d1
wpt-pr: 43096

UltraBlame original commit: 98ddfdc5d64d44b5c70b46671958a3cd76fa988a
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

No branches or pull requests

8 participants