-
Notifications
You must be signed in to change notification settings - Fork 1
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
🔊 #383
Conversation
Reviewer's Guide by SourceryThis pull request focuses on cleaning up the code by removing a commented-out line and adding a debug log statement to improve traceability of the 'library' variable in the '_create_client' method. File-Level Changes
Tips
|
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 @mraniki - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 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 to tell me if it was helpful.
library = ( | ||
kwargs.get("library") | ||
or kwargs.get("platform") | ||
or kwargs.get("protocol") | ||
or kwargs.get("parser_library") | ||
or "standard" | ||
) | ||
logger.debug("Library: {}", library) |
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: Use f-string for logging
Consider using an f-string for the logging statement to maintain consistency and readability. For example: logger.debug(f"Library: {library}")
.
logger.debug("Library: {}", library) | |
logger.debug(f"Library: {library}") |
@@ -111,14 +111,14 @@ def _create_client(self, **kwargs): | |||
library is not supported. | |||
|
|||
""" | |||
# library = kwargs.get("parser_library", "standard") |
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: Remove commented-out code
Consider removing the commented-out line entirely if it is no longer needed. Keeping commented-out code can clutter the codebase and reduce readability.
# library = kwargs.get("parser_library", "standard") | |
library is not supported. | |
""" | |
library = ( | |
kwargs.get("library") |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #383 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 6 6
Lines 137 138 +1
=========================================
+ Hits 137 138 +1 ☔ View full report in Codecov by Sentry. |
Summary by Sourcery
This pull request enhances the _create_client method by adding a debug log statement to output the selected library, aiding in debugging and monitoring.