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
Use ruff format instead of black #1680
base: pre/2.7
Are you sure you want to change the base?
Conversation
Hi Yannick, thanks for this! Everything you're suggesting sounds great, and thanks for finding out these bugs. I guess we'd have to check/sync this with the backend too as they are using black to format - maybe we can migrate it to ruff. There was some chat about doing this between releases too, ultimately it'd be nice to just have the PRs with all the lint changes on both repos ready to merge after a corresponding release. Tyler can say more but maybe the next major release might not be imminent exactly so wonder if we want to apply a solution sooner. |
I think this should be fine separately from the backend (we can migrate that to ruff too whenever we want) right @daquinteroflex? I mean, on the backend we only black specific folders, not the submodules. |
@yaugenst-flex do we want to just use ruff 0.3? I dont think we're tied to a specific |
Nice 👍
Is there a way to replicate this using
All three sound good to me |
- repo: https://github.com/astral-sh/ruff-pre-commit | ||
rev: "v0.3.2" | ||
rev: "v0.2.2" |
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.
so we need to make sure this always matches the version in the pyproject.toml
?
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.
they should match yeah, otherwise the lint errors that you get in your IDE (which uses the environment's ruff) might not match the ones that pre-commit emits (which uses its own ruff)
i'm all for it, i was just a bit scared of reformatting pretty much the whole codebase. if we're not too worried about that i'll just switch to the latest ruff (
not directly (since ruff has no python api), but i think i can come up with something |
The biggest issue with reformatting the whole codebase is if it will make currently open PRs annoying to rebase. In principle it seems like it is not too bad, but we've been postponing linting all of the backend code until 2.7 is merged into develop (e.g. in preparation for the official release). Maybe best to do that then, too? |
Ok yeah that makes sense, let's defer it until then. This PR is pretty much ready to go, I'll rebase and update ruff once everything else is done. |
ok. BTW I'll tag this as |
Yeah, that's true! I guess was specifically thinking mainly in terms of avoiding conflicting |
I mean, it kinda sounds like we may keep our life simple if we either switch both already, or wait until after the backend poetry refactor. I don't really see a big need to swtich so I'm fine either way (or even switching the frontend only for now if you don't think it will create headaches) |
Yeah, makes sense to do it separately sooner. I'll modify the existing linting PR on the backend to this version so we can merge it to the current backend whenever this front-end one is merged too. |
Ok, yeah, let's do it after merging pre/2.7 to develop |
Just a note:
|
I think the only way to do that is if we use a custom lint config for the tests (which is fine and maybe we should do that anyway). This easy to add. What do you think? |
I think it's worth it if it's not too painful. Cuts down on the amount of random failing tests we're going to have to deal with in the future |
@yaugenst-flex @daquinteroflex when is the best time to merge this? should we do it soon-ish? |
After we merge 2.7 branches to develop - which I'm not opposed to doing soon. But it may be simpler to first merge the last remaining big things, adjoint, eme, subpixelspec. |
Closes #1662
I tried to keep these changes as minimal as possible so that we don't re-format the whole repo in a minor release. Following changes:
ruff 0.3.2 -> 0.2.2
, as this is what most closely matches ourblack
version (23.12.1
).ruff >0.3
already partly implements the 2024 code style.docs/faq
anddocs/notebooks
, as these seem to be implicitly ignored currently (see A few improvements tidy3d-notebooks#6). We should work toward not excluding them, but not for2.7
I guess.tidy3d/
were changed from the previous formatting. There are two changes in line breaks, the rest are just trailing commas.black
for its Python API inmake_script.py
, and I didn't want to remove this for2.7
.fix = false
by default, as we set--fix
in the pre-commit hook, and lint fixing during development should be left to the developer's IDE config imotarget-version
frompy310
topy39
as that is the minimal Python version that we support. The linting should not target a newer Python version as that can lead to errors. It was also misconfigured (and ignored) previously, as it somehow ended up in thebumpmyversion
config.ruff
section inpyproject.toml
up into[tool.ruff]
and[tool.ruff.lint]
.Overall I think this simplifies the workflow a bit and we also get a nice 4x speedup:
Down the line it would be nice to update to a later version of
ruff
, but that will involve adopting at least the 2024 code style, which will reformat the repo quite a bit, so this should probably be done in a major release.@tylerflex I think the best way to handle pytest fixtures being detected as missing imports is to just be explicit about it, i.e. using
# noqa: F401
. This issue has been raised on bothblack
andruff
before and the consensus seems to be that no one wants to implementpytest
-specific logic for this case. The alternative would be to excludeF401
module- or even linter-wide, but I think that's probably a bad idea.@momchil-flex Could you perhaps check if these changes are compatible with the backend?