Skip to content

Conversation

@NotTheEvilOne
Copy link
Contributor

@NotTheEvilOne NotTheEvilOne commented Nov 25, 2025

What this PR does / why we need it:
This PR switches the Python code linting tool from pyright to mypy based on pre-commit. It additionally replaces black with ruff again based on pre-commit. Based on these tools changes to the underlying Python code has been integrated.

The switch from pyright is based on the author's opinion that the linting tool produces way more false-positives than mypy which is more configurable regarding typing of third-party Python packages as well. This issue made it more difficult to meet expected code changes expected by pyright and therefore a higher burden to provide changes to the code base.

Code formatting in ruff on the other hand feels more Pythonic than what black is enforcing in the configuration we used.

Which issue(s) this PR fixes:
Related #165
Requires #248

@NotTheEvilOne NotTheEvilOne self-assigned this Nov 25, 2025
Copy link
Contributor

@vivus-ignis vivus-ignis left a comment

Choose a reason for hiding this comment

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

While I'm two hands for getting rid of pyright, this PR doesn't seem to be in line with the title and concerns many other topics too. Could you split it into smaller focused parts?

@NotTheEvilOne
Copy link
Contributor Author

While I'm two hands for getting rid of pyright, this PR doesn't seem to be in line with the title and concerns many other topics too. Could you split it into smaller focused parts?

Thanks for your feedback. As mentioned in the description this PR requires #248. With that being merged the changes are only focused on typing changes and pyright replacement.

@vivus-ignis
Copy link
Contributor

While I'm two hands for getting rid of pyright, this PR doesn't seem to be in line with the title and concerns many other topics too. Could you split it into smaller focused parts?

Thanks for your feedback. As mentioned in the description this PR requires #248. With that being merged the changes are only focused on typing changes and pyright replacement.

You mean this PR contains changes that are going to be merged in another PR?

@NotTheEvilOne
Copy link
Contributor Author

While I'm two hands for getting rid of pyright, this PR doesn't seem to be in line with the title and concerns many other topics too. Could you split it into smaller focused parts?

Thanks for your feedback. As mentioned in the description this PR requires #248. With that being merged the changes are only focused on typing changes and pyright replacement.

You mean this PR contains changes that are going to be merged in another PR?

That's correct. As both PRs #248 and this one change the same files it's not possible to separate them in a useful way.

@NotTheEvilOne NotTheEvilOne force-pushed the feature/replace-pyright-with-pre-commit branch from a8f18a1 to 86e9193 Compare November 25, 2025 14:46
Copy link
Contributor

@vivus-ignis vivus-ignis left a comment

Choose a reason for hiding this comment

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

Ship it

@NotTheEvilOne NotTheEvilOne force-pushed the feature/replace-pyright-with-pre-commit branch from 86e9193 to efd4075 Compare November 25, 2025 15:02
@codecov
Copy link

codecov bot commented Nov 25, 2025

Codecov Report

❌ Patch coverage is 92.18107% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.40%. Comparing base (d64300c) to head (212b2d6).
⚠️ Report is 17 commits behind head on main.

Files with missing lines Patch % Lines
src/gardenlinux/features/__main__.py 68.75% 10 Missing ⚠️
src/gardenlinux/features/cname.py 77.77% 6 Missing ⚠️
src/gardenlinux/oci/__main__.py 85.71% 1 Missing ⚠️
src/gardenlinux/oci/manifest.py 93.33% 1 Missing ⚠️
src/gardenlinux/s3/s3_artifacts.py 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #270      +/-   ##
==========================================
+ Coverage   90.63%   91.40%   +0.77%     
==========================================
  Files          42       42              
  Lines        2008     2037      +29     
==========================================
+ Hits         1820     1862      +42     
+ Misses        188      175      -13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@NotTheEvilOne NotTheEvilOne force-pushed the feature/replace-pyright-with-pre-commit branch from efd4075 to ba27878 Compare November 25, 2025 15:05
Signed-off-by: Tobias Wolf <[email protected]>
On-behalf-of: SAP <[email protected]>
@NotTheEvilOne NotTheEvilOne force-pushed the feature/replace-pyright-with-pre-commit branch from ba27878 to 212b2d6 Compare November 25, 2025 15:06
@ByteOtter
Copy link
Contributor

Just checking in because I got the notification.
Sorry, but I don't really get this PR. The description is confusing to say the least. pyright is not a formatter. It's a type checker. pre-commit isn't a formatter neither. That's a tool for running checks before a commit. looks like you introduced ruff now as a formatter/linter? I don't necessarily disagree with that, I just don't really see what benefit this brings given that we already had black as a formatter which was consistent with the other projects as well, and pyright as de-factor industry default type checker?
Also, ruff does not do perform type checking like pyright does. Please keep that in mind. There are a bunch of things ruff does not catch, that pyright will.

Can you clear up for me what benefit this setup will have over the black + pyright approach?

@NotTheEvilOne
Copy link
Contributor Author

Just checking in because I got the notification. Sorry, but I don't really get this PR. The description is confusing to say the least. pyright is not a formatter. It's a type checker. pre-commit isn't a formatter neither. That's a tool for running checks before a commit. looks like you introduced ruff now as a formatter/linter? I don't necessarily disagree with that, I just don't really see what benefit this brings given that we already had black as a formatter which was consistent with the other projects as well, and pyright as de-factor industry default type checker? Also, ruff does not do perform type checking like pyright does. Please keep that in mind. There are a bunch of things ruff does not catch, that pyright will.

Can you clear up for me what benefit this setup will have over the black + pyright approach?

For pre-commit another configuration used is for mypy (static type checker). pyright is overacting and not really catching all code paths correctly. E.g. it is throwing errors for None values used in string manipulation (which is fine per se) but does not correctly identify that its value is verified in a if condition beforehand. IMHO pyright produces way more false-positives than mypy which is more configurable regarding typing of third-party Python packages as well.

Code formatting in ruff on the other hand is more Pythonic than what black is enforcing in the configuration we used.

That being said I find the code now formatted and verified with the tools used way more readable in some corner cases (asserts, prefering inline functions instead `lambda etc.).

Copy link
Contributor

@yeoldegrove yeoldegrove left a comment

Choose a reason for hiding this comment

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

@NotTheEvilOne I am fine with the changes but as @ByteOtter pointed out, the PR title and description is missing important context.
Could you please simply sum at what you commented and update the description? What is removed, what is added and why?

@NotTheEvilOne NotTheEvilOne changed the title Replace pyright with pre-commit Change Python code formatting and linting tools Dec 4, 2025
@NotTheEvilOne
Copy link
Contributor Author

@NotTheEvilOne I am fine with the changes but as @ByteOtter pointed out, the PR title and description is missing important context. Could you please simply sum at what you commented and update the description? What is removed, what is added and why?

Done

@yeoldegrove yeoldegrove self-requested a review December 4, 2025 10:33
@ByteOtter
Copy link
Contributor

Thanks for updating the descriptions this makes it more clear why this is happening. I already laid out my position, but in the end if it's a popular decision go ahead. However, I would like to point out that we should then also switch @yeoldegrove's tooling for gardenlinux' test-ng as well so we stay consistent. (Add the benefit of not having to maintain separate IDE configs for two places)

@yeoldegrove
Copy link
Contributor

However, I would like to point out that we should then also switch @yeoldegrove's tooling for gardenlinux' test-ng as well so we stay consistent. (Add the benefit of not having to maintain separate IDE configs for two places)

Good point!

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.

5 participants