Add multiple modules based on a template module and quantity #18
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Type: Enhancement
PR Summary: This pull request introduces a method for creating multiple modules based on a template module and a specified quantity. It aims to improve the clarity of CAN messages by allowing for the efficient generation of repeated modules, which is particularly useful for modules like MCC that have repeated boards.
Decision: Comment
📝 Type: 'Enhancement' - not supported yet.
- Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
✅ Small diff: the diff is small enough to approve with confidence.
No details provided.
General suggestions:
- Ensure comprehensive testing for the new
add_multiple_modulesmethod, including edge cases and validation against conflicts in module signatures and topic IDs. - Consider adding more detailed documentation or comments within the code to explain the logic and usage of the
add_multiple_modulesmethod, especially how it handles unique naming and ID incrementation. - Review the error handling and validation logic within the
add_multiple_modulesmethod to ensure that it robustly prevents the creation of modules with conflicting identifiers.
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? ✨
| @@ -70,6 +71,43 @@ | |||
| "frequency": self.frequency, | |||
| "frame_length": self.frame_length | |||
| } | |||
|
|
|||
| @classmethod | |||
There was a problem hiding this comment.
suggestion (llm): It's great to see the implementation of the from_dict class method for the Topic class. This method will significantly ease the creation of Topic instances from dictionaries, especially useful in the context of dynamically generating topics. However, it would be beneficial to add unit tests for this method to ensure its correct functionality and to handle edge cases properly.
|
|
||
| self.modules.append(module.get()) | ||
|
|
||
| def add_multiple_modules(self, module: Module, quantity: int) -> None: |
There was a problem hiding this comment.
suggestion (llm): The addition of the add_multiple_modules method is a crucial feature for generating repeated modules efficiently. However, there's a need for comprehensive unit tests covering this method to validate the creation of multiple modules with unique names, signatures, and updated topic IDs. Tests should also verify that the correct number of modules is added and that their properties are correctly incremented.
|
|
||
| self.modules.append(module.get()) | ||
|
|
||
| def add_multiple_modules(self, module: Module, quantity: int) -> None: |
There was a problem hiding this comment.
issue (llm): While the add_multiple_modules method is a significant addition, it's important to ensure that the new module signatures and topic IDs do not conflict with existing ones in the CAN database. Consider adding validation logic to prevent such conflicts, which could lead to issues in message identification and handling.
Some modules, such as MCC, have repeated boards. Implementing a method to create repeated modules will improve the clarity of CAN messages.