Fix server response parsing #221
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #221 +/- ##
==========================================
+ Coverage 88.55% 88.90% +0.35%
==========================================
Files 11 11
Lines 559 559
==========================================
+ Hits 495 497 +2
+ Misses 64 62 -2 ☔ View full report in Codecov by Sentry. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leshchenko1979 - I've reviewed your changes and they look great!
General suggestions:
- Consider further modularization of the response parsing logic to facilitate future enhancements.
- Review the consistency of method documentation to ensure clarity across the codebase.
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Docstrings: all looks good
Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨
| def generate_tasks(self): | ||
| """Group items in batches and create asyncio tasks for each batch""" | ||
|
|
||
| batches = ( | ||
| { | ||
| "halt": 0, | ||
| "cmd": { | ||
| self.batch_command_label( | ||
| i, item | ||
| ): f"{self.method}?{http_build_query(item)}" | ||
| for i, item in enumerate(next_batch) | ||
| }, | ||
| } | ||
| for next_batch in chunked(self.item_list, BITRIX_MAX_BATCH_SIZE) | ||
| self.package_batch(chunk) | ||
| for chunk in chunked(self.item_list, BITRIX_MAX_BATCH_SIZE) |
There was a problem hiding this comment.
suggestion (code_refinement): Refactoring generate_a_task to generate_tasks improves readability and intent clarity.
| def package_batch(self, chunk): | ||
| return { | ||
| "halt": 0, | ||
| "cmd": { | ||
| f"cmd{i:010}": f"{self.method}?{http_build_query(item)}" | ||
| for i, item in enumerate(chunk) | ||
| }, | ||
| } |
There was a problem hiding this comment.
suggestion (code_refinement): Extracting batch packaging logic into package_batch method enhances modularity.
| extracted = self.extract_from_single_response(self.result["result"]) | ||
|
|
||
| return extracted[0] if isinstance(extracted, list) else extracted |
There was a problem hiding this comment.
suggestion (code_refinement): Simplifying the return logic for single responses enhances code readability.
| return isinstance(self.result, dict) and "result" in self.result | ||
|
|
||
| @staticmethod | ||
| def extract_from_single_response(result: dict): | ||
| def extract_from_single_response(self, result: dict): | ||
| # если результат вызова содержит только словарь {ключ: список}, |
There was a problem hiding this comment.
suggestion (code_refinement): Removing static method in favor of instance method allows for more flexible future extensions.
No description provided.