-
-
Notifications
You must be signed in to change notification settings - Fork 65
Fixes and style #425
Fixes and style #425
Conversation
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 @juancarlospaco - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 5 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| if self.headers.get( | ||
| "Accept" | ||
| ) and "application/vnd.pgrst.plan" in self.headers.get("Accept"): | ||
| if not "+json" in self.headers.get("Accept"): |
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.
suggestion (code_refinement): Improved readability with 'not in' for membership test.
| if not "+json" in self.headers.get("Accept"): | |
| if "+json" not in self.headers.get("Accept"): |
| raise APIError(r.json()) | ||
| except ValidationError as e: | ||
| raise APIError(r.json()) from e | ||
| except JSONDecodeError as e: |
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.
suggestion (code_refinement): Removal of unused exception variable 'e' is a good practice.
| except JSONDecodeError as e: | |
| except JSONDecodeError: | |
| raise APIError(generate_default_error_message(r)) |
| raise APIError(r.json()) | ||
| except ValidationError as e: | ||
| raise APIError(r.json()) from e | ||
| except JSONDecodeError as e: |
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.
suggestion (code_refinement): Good removal of the unused variable 'e' in exception handling.
| ) -> Self: | ||
| try: | ||
| data = request_response.json() | ||
| except JSONDecodeError as e: |
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.
suggestion (code_refinement): Efficient use of exception handling by removing unused variable 'e'.
| except JSONDecodeError as e: | |
| except JSONDecodeError: | |
| raise APIError(generate_default_error_message(r)) |
| if self.headers.get( | ||
| "Accept" | ||
| ) and "application/vnd.pgrst.plan" in self.headers.get("Accept"): | ||
| if not "+json" in self.headers.get("Accept"): |
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.
suggestion (code_refinement): Consistency in using 'not in' for membership checks enhances code readability.
| if not "+json" in self.headers.get("Accept"): | |
| if "+json" not in self.headers.get("Accept"): |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #425 +/- ##
==========================================
+ Coverage 95.72% 95.91% +0.18%
==========================================
Files 28 28
Lines 1639 1715 +76
==========================================
+ Hits 1569 1645 +76
Misses 70 70 ☔ View full report in Codecov by Sentry. |
What kind of change does this PR introduce?