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

Align codestyle #124

Merged
merged 8 commits into from
Sep 25, 2023
Merged

Align codestyle #124

merged 8 commits into from
Sep 25, 2023

Conversation

gi0baro
Copy link
Member

@gi0baro gi0baro commented Sep 7, 2023

Close #123

Copy link
Contributor

@matthiask matthiask left a comment

Choose a reason for hiding this comment

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

I think this would make a rework of #119 necessary, but that's definitely fine since I still have problems with maturin.

My personal preference is to not install formatters using [project.optional-dependencies] but only referencing them through the pre-commit configuration, for example https://github.com/matthiask/django-content-editor/blob/944b9b8b403ecdcfe520cd088569f52259f57fdd/.pre-commit-config.yaml#L25-L32 because I can reuse the installations between many projects and I do not have to install black / ruff / etc. once for each project, but whatever works.

(I won't get into the specifics of code formatting, that's not an useful discussion. Only this: I try configuring tools as little as possible, but no less. The important part is actually using formatters and linters.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see now. It came from the fellow Pydantic’s configuration. I wasn’t sure why these particular choices.

I don’t know why Samuel and the contributors went against Black a little. But I don’t think we have any legacy constraints here to break Black’s rules.

As a different example, we could look at the Structlog’s configuration. It’s made differently in both Ruff and Black rules.

We can do something in the middle.

Also, we can expand optional dependencies to give a choice to install linters or do it another way.

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated
Comment on lines 77 to 82
[tool.black]
color = true
line-length = 120
target-version = ['py38', 'py39', 'py310', 'py311']
skip-string-normalization = true
skip-magic-trailing-comma = true
Copy link
Contributor

@Aeron Aeron Sep 14, 2023

Choose a reason for hiding this comment

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

If we don’t want to break or skip the Black’s rules, then it’ll be enough to have line length and target versions:

Suggested change
[tool.black]
color = true
line-length = 120
target-version = ['py38', 'py39', 'py310', 'py311']
skip-string-normalization = true
skip-magic-trailing-comma = true
[tool.black]
line-length = 120
target-version = ['py38', 'py39', 'py310', 'py311']

Copy link

@lautjy lautjy Sep 24, 2023

Choose a reason for hiding this comment

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

2 cents: The closer configs are to black defaults the better it is.

@gi0baro gi0baro marked this pull request as ready for review September 25, 2023 13:59
@gi0baro
Copy link
Member Author

gi0baro commented Sep 25, 2023

@matthiask @Aeron @lautjy thank you all for your comments and feedbacks.
I should have managed to add the majority of your suggestions; regarding black I really prefer to have single quote strings (you type 'em faster) and thus leave the thing to ruff, while on the pre-commit thing I'd like to think about it in a separated discussion.

I'm merging this right now, as in the next few days I'll be AFK, and at least existing open PRs can solve eventual conflicts. I'm obviously open to further enhancements following this PR :)

@gi0baro gi0baro merged commit c95fd0c into master Sep 25, 2023
11 of 13 checks passed
@gi0baro gi0baro deleted the align-codestyle branch September 25, 2023 16:00
@lautjy
Copy link

lautjy commented Sep 25, 2023

Your choice is valid. Just want to add that you could write ' or ", and if you have "format on save" in your IDE, it would run black and automatically convert it to ". So having the default config doesn't force you to change habits - and you will quickly get used to seeing the code (have been thru the change years ago).
IMHO the more repos in default black the better. But at this point we are veering too close to nitpicking. Your repo, your choice :)

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.

Python code formatting and linting
4 participants