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

Apply black code formatter to the python codebase #286

Merged
merged 3 commits into from
Sep 26, 2024

Conversation

youtux
Copy link
Contributor

@youtux youtux commented Sep 21, 2024

🤔 What's changed?

  • Apply black code formatter to the python codebase, to enforce a standard formatting
  • Add a .pre-commit-config.yaml to the project, so that we can enforce this convention
  • TODOs

⚡️ What's your motivation?

The python codebase does not really use a standard formatting, this is arguably widely accepted as "annoying".

🏷️ What kind of change is this?

  • 🏦 Refactoring/debt/DX (improvement to code design, tooling, etc. without changing behaviour)

♻️ Anything particular you want feedback on?

📋 Checklist:

  • I agree to respect and uphold the Cucumber Community Code of Conduct
  • I've changed the behaviour of the code
    • I have added/updated tests to cover my changes.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • Users should know about my change
    • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

This text was originally generated from a template, then edited by hand. You can modify the template here.

@youtux youtux marked this pull request as ready for review September 21, 2024 08:29
Copy link
Contributor

@mpkorstanje mpkorstanje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that you are adding pre-commit hooks. Are these setup in such a way that they will not interfere with contributors who make contributions without python installed?

# pre-commit's default_language_version, see
# https://pre-commit.com/#top_level-default_language_version
language_version: python3.8
exclude: "python/gherkin/parser.py"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally this is kept in the python folder.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in f1e0e7e

@youtux youtux changed the base branch from main to python-add-type-annotations September 21, 2024 16:51
Base automatically changed from python-add-type-annotations to main September 22, 2024 12:16
@youtux
Copy link
Contributor Author

youtux commented Sep 22, 2024

I see that you are adding pre-commit hooks. Are these setup in such a way that they will not interfere with contributors who make contributions without python installed?

yes

by running `pre-commit run --all-files` and reverting `parser.py`
Copy link
Contributor

@jsa34 jsa34 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@jsa34
Copy link
Contributor

jsa34 commented Sep 22, 2024

I'm assuming the GitHub actions work will be in separate PR

@luke-hill
Copy link
Contributor

One thing to bare in mind (I'm doing the same in ruby), is some long methods in gherkin are written in a way that is almost language agnostic, so it reads the same in every language.

See here: https://github.com/cucumber/gherkin/blob/main/CONTRIBUTING.md#consistency-between-implementations

@@ -0,0 +1,9 @@
# See https://pre-commit.com for more information
# See https://pre-commit.com/hooks.html for more hooks
repos:
Copy link
Contributor

@mpkorstanje mpkorstanje Sep 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to keep this whole file in the python folder?

Right now, if I have the pre-commit plugin installed, it looks like it will activate black regardless of whether or not I have python.

This poses a problem for contributors seeking to contribute to languages other than python, they may not have it installed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pre-commit installs hooks in a completely isolated environment that does not affect global packages. If other contributors don't touch the python folder, the black hook won't even run at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried putting .pre-commit-config.yaml under the python directory, but it won't allow any other .pre-commit-config.yaml to be used if anyone decides to have one under java/ or any other folder.
Seems like a limitation of pre-commit.
So the only way is to keep it on the root dir, and explicitly whitelist the dirs that each hook should handle.

Copy link
Contributor

@mpkorstanje mpkorstanje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about the pre-commit hook yet. Looks like it will activate regardless.

@youtux
Copy link
Contributor Author

youtux commented Sep 26, 2024

One thing to bare in mind (I'm doing the same in ruby), is some long methods in gherkin are written in a way that is almost language agnostic, so it reads the same in every language.

not sure how this applies in this PR @luke-hill

@youtux youtux merged commit 465b971 into main Sep 26, 2024
13 checks passed
@youtux youtux deleted the python-use-black-style branch September 26, 2024 20:24
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

Successfully merging this pull request may close these issues.

4 participants