Skip to content

Conversation

rnestler
Copy link
Member

@rnestler rnestler commented Jun 3, 2024

This is just a quick PR to answer the following questions:

  • Should we add a code format check to CI? Yes. Done.
  • Is the configuration like this OK? Yes. Done.
  • Should we reformat the whole repo? Yes. Done.
  • Should we also replace flake8 with ruff? Yes. Done.

@rnestler rnestler requested review from dbrgn and ubruhin and removed request for dbrgn June 3, 2024 10:34
Copy link
Member

@ubruhin ubruhin left a comment

Choose a reason for hiding this comment

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

I have no clear opinion about this. Generally I like consistent and enforced formatting and also the style looks quite good to me. But especially some configuration lists (e.g. FetConfig or ChipConfig) are getting more verbose / harder to read. Though not too bad I guess 🤔 Anyway, I would prefer a shorter line length.

Btw I never hear of ruff, so no opinions about that tool 😉

@dbrgn
Copy link
Member

dbrgn commented Jun 24, 2024

I'm a fan of autoformatters, as long as they allow sufficient control and can be disabled for certain parts of the code if necessary. So +1 from my side for ruff.

@z3ugma z3ugma mentioned this pull request Jul 7, 2025
@rnestler rnestler force-pushed the use-ruff-for-formatting branch from 7fe5e73 to e4555bc Compare July 11, 2025 11:57
@rnestler rnestler force-pushed the use-ruff-for-formatting branch from e1c1399 to 861e40e Compare July 11, 2025 12:28
@rnestler rnestler force-pushed the use-ruff-for-formatting branch from 861e40e to 2d131f3 Compare July 11, 2025 12:38
@rnestler rnestler requested review from ubruhin and dbrgn July 11, 2025 12:50
I tried to match the existing isort config as close as possible.
@rnestler rnestler force-pushed the use-ruff-for-formatting branch from 2d131f3 to 35b5125 Compare July 11, 2025 12:51
@z3ugma
Copy link

z3ugma commented Jul 22, 2025

Are you encouraging users to switch to uv as well? Using [project.optional-dependencies] and having a [test] group installed with pip is a little awkward compared to using dev dependencies https://docs.astral.sh/uv/concepts/projects/dependencies/#dependency-groups

for example:

librepcb_parts_generator % uv sync --group test
Resolved 25 packages in 50ms
error: Group `test` is not defined in the project's `dependency-groups` table

as opposed to moving to this in pyproject.toml:

[dependency-groups]
dev = ["pytest", "mypy == 1.10.0", "ruff ~= 0.12.2"]
librepcb_parts_generator % uv sync --group dev 
Resolved 25 packages in 49ms
Prepared 3 packages in 1.69s
Installed 3 packages in 17ms
 + mypy==1.10.0
 + mypy-extensions==1.1.0
 + ruff==0.12.4

@ubruhin
Copy link
Member

ubruhin commented Jul 23, 2025

Are you encouraging users to switch to uv as well?

I'd suggest to do so, and adjust pyproject.toml & README.md accordingly, but I'd prefer getting this PR merged first, and then do the other updates. It would be nice if this PR was merged as soon as possible to not create conflicts with other updates.

@rnestler Do you intend to add some fmt: off/on markers to fix the verbose formatting of config blocks? Or do you want me to do it?

@z3ugma
Copy link

z3ugma commented Aug 1, 2025

@rnestler have you got time to push this forward? I rebased ahead of these changes for #145

@z3ugma
Copy link

z3ugma commented Aug 6, 2025

@rnestler can you progress this Ruff formatting PR so that I can merge my changes that are behind it? Or let me know if you're busy and want to me take this over

@ubruhin
Copy link
Member

ubruhin commented Aug 6, 2025

Or let me know if I should take it over @rnestler
I think only some #fmt: off/on need to be added before running the formatter.

EDIT: Ah I didn't see @z3ugma also offered to take it over 🙈

@rnestler
Copy link
Member Author

rnestler commented Aug 6, 2025

Sorry for radio silence 🙈 I was on holidays the last two weeks.

@rnestler
Copy link
Member Author

rnestler commented Aug 6, 2025

@z3ugma If you have time and capacity you can take over, thanks for the offer 🙂

@ubruhin
Copy link
Member

ubruhin commented Sep 2, 2025

Due to the silence again, I have now applied all the #fmt: off/on by myself and will merge it soon.

@ubruhin ubruhin merged commit b80e125 into master Sep 3, 2025
12 checks passed
@ubruhin ubruhin deleted the use-ruff-for-formatting branch September 3, 2025 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants