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.

Sign up for GitHub

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

Jump to bottom

Format code with Ruff #221

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Format code with Ruff #221

wants to merge 1 commit into from

Conversation

JonathanPlasse
Copy link
Sponsor Contributor

@JonathanPlasse JonathanPlasse commented May 19, 2024 •
edited

Purpose of the pull request

Description about the pull request

  • Add ruff format check in CI
  • Add ruff config in pyproject.toml (line-length of 88, with Python 3.8 minimal version).
  • Add # fmt: skip where we want to keep manual formatting (e.g. Blender version)
  • Upgrade Ruff to the 0.4.4 version

@JonathanPlasse
Copy link
Sponsor Contributor Author

@nutti, would you prefer a line-length other than 88?
Is there any other part of the code, you would like to keep manually formatting?
I would like to merge this as soon as possible to avoid merge conflict and having to rebase.

@nutti
Copy link
Owner

nutti commented May 19, 2024 •
edited

@JonathanPlasse

Thank you for your continuous work. I will check them.
However, I can not review this PR quickly because I will be busy next week.

@JonathanPlasse
Copy link
Sponsor Contributor Author

No problem, as long as it is the first PR you merge, it is fine as there will not be any merge conflict.

@JonathanPlasse
Copy link
Sponsor Contributor Author

Thank you for being active, I am really glad seeing this project moving forward and being part of it.

This was referenced May 19, 2024
Merged
Merged
nutti
nutti requested changes May 26, 2024
View reviewed changes
Copy link
Owner

@nutti nutti left a comment

Choose a reason for hiding this comment

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

@JonathanPlasse

Sorry for the late review.
I left some comments.

.github/workflows/lint.yml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
src/fake_bpy_module/generator/translator.py Outdated Show resolved Hide resolved
src/fake_bpy_module/transformer/data_type_refiner.py Outdated Show resolved Hide resolved
src/fake_bpy_module/transformer/data_type_refiner.py Outdated Show resolved Hide resolved
src/gen.py Show resolved Hide resolved
tests/python/fake_bpy_module_test/fake_bpy_module_test/utils_test.py
@@ -114,7 +115,8 @@ def test_append_child(self):
<return>
<description>
<data-type-list>
""")
""",
Copy link
Owner

Choose a reason for hiding this comment

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

Is this comma needed?

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

No, but the Ruff formatter cannot be configured not to add it.

Copy link
Owner

Choose a reason for hiding this comment

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

I think adding a comma at the last of the parameters makes us difficult to distinguish between set and the function parameter.
How do you think about it?

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

A comma is added automatically when there are more than one parameter.
The purpose of this rule is to minimize the diff in commits when adding or removing or switching parameters.
I am not bothered by it.
I prefer having an auto-formatter than not at all.
This allows focusing on code instead of formatting text manually.
This is really useful when format run on save.
Even if the style is less good.

Copy link
Owner

Choose a reason for hiding this comment

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

OK. I understood your idea.
However, adding comma is not intuitively for the reader perspective.
Is it possible to disable this format even if other commas (including list, set, and so on) do not work?

Copy link
Owner

@nutti nutti May 28, 2024 •
edited

Choose a reason for hiding this comment

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

I think configuration file can select it like sphinx project.
https://github.com/sphinx-doc/sphinx/blob/master/.ruff.toml
https://docs.astral.sh/ruff/rules/trailing-comma-on-bare-tuple/
https://docs.astral.sh/ruff/configuration/

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

This is a linter rule not a formatter config.
The linter and formatter of Ruff are independent.
They do not influence each other.

Copy link
Owner

Choose a reason for hiding this comment

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

In my understanding, this PR targets to introduce Ruff linter not format the code.
Please consider to filter the error code by using the configuration file.

We could not accept this PR if it affects the readability.
It is important to keep the readability more than usefulness who does not introduce Ruff.

Copy link
Owner

Choose a reason for hiding this comment

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

This should be configurable from the editor.
I would like to filter the error to meet this project before merging this PR.

I found there is a format configuration. This doesn't work in your environment?
https://github.com/astral-sh/ruff-vscode?tab=readme-ov-file#configuring-ruff

Copy link
Owner

Choose a reason for hiding this comment

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

This is a formatter configuration.
https://docs.astral.sh/ruff/formatter/#configuration
I think skip-magic-trailing-comma can solve this issue.
https://docs.astral.sh/ruff/settings/#format_skip-magic-trailing-comma

Maybe, we should consider other rules as well.

tests/python/import_module_test/run_tests.py Show resolved Hide resolved
nutti
nutti reviewed May 27, 2024
View reviewed changes
src/fake_bpy_module/analyzer/directives.py Outdated Show resolved Hide resolved
src/fake_bpy_module/support.py Outdated Show resolved Hide resolved
src/fake_bpy_module/support.py Outdated Show resolved Hide resolved
src/gen.py Show resolved Hide resolved
nutti
nutti requested changes May 27, 2024
View reviewed changes
Copy link
Owner

@nutti nutti left a comment

Choose a reason for hiding this comment

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

Thanks @JonathanPlasse for updating this PR.
I have an additional comments.

@nutti
Copy link
Owner

nutti commented May 27, 2024

@JonathanPlasse

Currently, build test for latest version will be failed.
I have already reported this issue and will be fixed in the tomorrow.
https://projects.blender.org/blender/blender/issues/122305

@nutti
Copy link
Owner

nutti commented May 29, 2024

@JonathanPlasse

I'm now experimenting the ruff linter.
After the experiment, I will propose a configuration file which matches this project.
Also we should follow same GitHub Action job as markdownlint and so on (by calling shell script instead of calling ruff command directly).

@JonathanPlasse
Copy link
Sponsor Contributor Author

Indeed, I do not think the Ruff formatter fit this project.
I will switch to the Ruff linter with formatting rules with auto-fix.

@nutti
Copy link
Owner

nutti commented May 29, 2024

Thank you for your consideration.
I think some ruff rules are useful for our project to clean our code, so I will adopt ruff linter to this project .

@nutti
Copy link
Owner

nutti commented Jun 1, 2024

@JonathanPlasse

This requires a time to finish.
Is it possible to pause this PR for now?
If I ready to share, I will ping you again.

@JonathanPlasse
Copy link
Sponsor Contributor Author

Yes, you can close the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nutti nutti nutti requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@JonathanPlasse JonathanPlasse

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@JonathanPlasse @nutti