-
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
✨ feature: adding action_identifier for each client in get_info #382
Conversation
Reviewer's Guide by SourceryThis pull request enhances the get_info method in findmyorder/main.py by adding an action_identifier for each client. The client_info string construction was updated to include this new attribute, providing more detailed information about each client. 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.
@@ -153,7 +153,10 @@ async def get_info(self): | |||
|
|||
""" | |||
version_info = f"ℹ️ {type(self).__name__} {__version__}\n" | |||
client_info = "".join(f"🔎 {client.name}\n" for client in self.clients) | |||
client_info = "".join( | |||
f"🔎 {client.name}\n🔠 {client.action_identifier}" |
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: Consider handling cases where client.action_identifier might be None or empty.
If client.action_identifier
can be None or an empty string, it might be better to handle these cases explicitly to avoid unexpected output.
f"🔎 {client.name}\n🔠 {client.action_identifier}" | |
f"🔎 {client.name}\n🔠 {client.action_identifier or 'N/A'}" |
client_info = "".join(f"🔎 {client.name}\n" for client in self.clients) | ||
client_info = "".join( | ||
f"🔎 {client.name}\n🔠 {client.action_identifier}" | ||
for client in self.clients |
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 (performance): Consider the performance impact of the join operation inside a generator expression.
While the current implementation is likely efficient for a small number of clients, if self.clients
can be large, it might be worth considering alternative approaches to improve performance.
for client in self.clients | |
client_info = [] | |
for client in self.clients: | |
client_info.append(f"🔎 {client.name}\n🔠 {client.action_identifier}") | |
client_info = "".join(client_info) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #382 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 6 6
Lines 137 137
=========================================
Hits 137 137 ☔ View full report in Codecov by Sentry. |
Summary by Sourcery
This pull request enhances the get_info method by including an action_identifier for each client, providing more detailed information.