-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Feat apply ruff #497
Feat apply ruff #497
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #497 +/- ##
==========================================
+ Coverage 94.70% 95.93% +1.22%
==========================================
Files 9 10 +1
Lines 680 861 +181
==========================================
+ Hits 644 826 +182
+ Misses 36 35 -1 ☔ View full report in Codecov by Sentry. |
9ccf571
to
b97646d
Compare
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.
Hi @phi-friday,
thanks for this PR! I think it would really improve this codebase. I would merge #498 before merging this, but I think reviews here might still take some time.
I've commented on some things that I noticed. These are not necessarily requests to change things, it assume there's generally a solid reason for you handling things the way you do that I might not have seen.
Consider this sort of a preliminary review, I would like to have a look again once you feel the PR is ready.
Finally, if there are no objections from you or any other contributors, I might ask a coworker with some more SE experience to additionally review this PR. I don't think my background here is sufficient to provide solid feedback.
Could you ping me when you feel this is ready for a review? |
@till-m |
001c79b
to
d305c1b
Compare
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.
Looks good to me, generally. I added some comments to clarify things.
@Panaetius Ralf, I would appreciate your feedback on this PR, if you have the time. No need to focus too much on the code (which should be functionally the same), the high-level things are what's important. |
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.
I think the codecov bot is not working correctly and coverage is not being updated?
In any case, I think this PR looks good, insofar as I am able to judge it.
I would wait a bit more to see if anyone else wants to give a review, but then I would merge this soon.
Thanks for your contribution! |
apply ruff example
related: #496