-
-
Notifications
You must be signed in to change notification settings - Fork 541
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
chore(ruff): Enable and fix all ruff rules that we can #3742
Conversation
Reviewer's Guide by SourceryThis pull request enables and fixes all applicable ruff rules, improving code style and maintainability. Some rules are ignored inline with "# noqa " where necessary, particularly in tests, to accommodate specific use cases. Class diagram showing key changes to error handling patternsclassDiagram
class BaseException {
+message: str
+raise_from(cause: Exception)
}
class CustomError {
+message: str
+__init__(message: str)
}
class ErrorWithPayload {
+payload: dict
+__init__(payload: dict)
}
BaseException <|-- CustomError
BaseException <|-- ErrorWithPayload
note for CustomError "Changed to use raise from pattern"
note for ErrorWithPayload "Changed to use None default for payload"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @bellini666 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Apollo Federation Subgraph Compatibility Results
Learn more: |
Hi, thanks for contributing to Strawberry 🍓! We noticed that this PR is missing a So as soon as this PR is merged, a release will be made 🚀. Here's an example of Release type: patch
Description of the changes, ideally with some examples, if adding a new feature. Release type can be one of patch, minor or major. We use semver, so make sure to pick the appropriate type. If in doubt feel free to ask :) Here's the tweet text:
|
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3742 +/- ##
==========================================
- Coverage 97.32% 97.28% -0.04%
==========================================
Files 501 502 +1
Lines 33386 33375 -11
Branches 5478 5477 -1
==========================================
- Hits 32493 32469 -24
- Misses 685 697 +12
- Partials 208 209 +1 |
CodSpeed Performance ReportMerging #3742 will not alter performanceComparing Summary
|
b9c0e39
to
275148b
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.
reviewed everything but tests, some comments are very opinionated
@@ -88,7 +87,6 @@ def get_type_by_name( | |||
raise NotImplementedError | |||
|
|||
@abstractmethod | |||
@lru_cache |
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.
just flagging this to come back & check the children later
@patrick91 @erikwrede thank you for the careful review :) (the PR is large lol) I reverted the changes for the rules that we don't want in the codebase and added them to the ignore list. Let me know if there's anything else you want me to revert 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.
lgtm now :) @patrick91 what do you think?
I went through all ruff rules we were ignoring and enabled all that made sense.
Some have legit use cases in some places, so I changed to an inline "# noqa " instead of having to disable those for the whole file. Some still made sense to be ignored on tests.
Summary by Sourcery
Enable more Ruff rules and fix existing violations.