-
Notifications
You must be signed in to change notification settings - Fork 108
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
Update format settings #1332
Update format settings #1332
Conversation
check the first commit for the settings |
[tool.black]
line-length = 179
[tool.ruff]
line-length = 179
indent-width = 4
target-version = "py39"
[tool.ruff.lint]
select = ["E", "F", "I"]
[tool.ruff.lint.pycodestyle]
max-doc-length = 179 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides a tiny comment, it lgtm!
I do wonder though, why raising the max line length? Any concrete reason?
@@ -118,20 +118,15 @@ replace = "[{new_version}] {now:%Y-%m-%d}" | |||
# ============================================================================ | |||
|
|||
[tool.black] | |||
# we should set this to 179 | |||
line-length = 120 | |||
line-length = 179 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a paragraph in the devguide that needs to be updated to reflect this change as well. And we need to create follow up issues to update this across the ecosystem (compas theme, and other core packages)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that line, I think the devguide needs to be update to indicate the move to ruff
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a paragraph in the devguide that needs to be updated to reflect this change as well. And we need to create follow up issues to update this across the ecosystem (compas theme, and other core packages)
am already doing this for compas_cgal
, compas_occ
, compas_libigl
, compas_gmsh
, compas_viewer
, compas_notebook
, compas_model
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re the devguide: yes indeed, we should. will make the change...
select = ["E", "F"] | ||
# we should remove this | ||
ignore = ["E501"] | ||
select = ["E", "F", "I"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without having to read the ruff docs, does this imply we enable all? What is excluded? Maybe drop a line indicating why/what is done here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is also "B", "Q", ... which are things we don't need/want
i just very often bump into the fact that 120 is too short. many comprehensions, or nested function calls get formatted really awkwardly because of it. also in docstrings it happens a lot (ruff doesn't properly differentiate those afaik). if you don't like it we can change it back :) |
nah, it's fine, I wasn't sure if there was an underlying technical reason for it (like ruff defaulting to it, or something like that) |
080eb4c
to
59466f6
Compare
What type of change is this?
Checklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.CHANGELOG.md
file in theUnreleased
section under the most fitting heading (e.g.Added
,Changed
,Removed
).invoke test
).invoke lint
).compas.datastructures.Mesh
.