allow merge_section to merge children classes #143
Conversation
Reviewer's Guide by SourceryThe PR modifies the merge_sections function to use section names instead of section definitions when handling sub-sections, making it more flexible for child classes. The change replaces direct section definition references with name-based lookups in all sub-section operations. Class diagram for merge_sections function updateclassDiagram
class Section {
+m_sub_section_count(name)
+m_get_sub_section(name, index)
+m_add_sub_section(name, sub_section)
}
class Update {
+m_def: Definition
+m_get_sub_sections(name)
+m_sub_section_count(name)
}
class Definition {
+all_sub_sections: Map
}
Section --> Definition
Update --> Definition
note for Section "Updated to use name instead of sub_section_def"
note for Update "Updated to use name instead of sub_section_def"
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @aalbino2 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please provide a more detailed description of the problem you're trying to solve. What issues were you encountering with child classes that required this change?
- Consider adding test cases that demonstrate both the problem with the current implementation and how your changes fix it
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 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 and I'll use the feedback to improve your reviews.
|
I guess as long as it works it should be good. Can you add a test case to the unit test which breaks with the old implementation and is fixed with the new one? |
|
Ah, I see now that the old tests don't run. I guess the first step would be to get those to run. |
There was a problem hiding this comment.
Nice idea! However, the if clause will need to be more involved. The current way of implementing might not work as section.m_def.all_base_sections points to a list of parent sections and does not contain the current section. I have added a suggestion to fix this.
But there's another issue. Most of the sections that will be used in merge_sections will always have common inherited sections like BaseSection or ArchiveSection. So the if will always be True even when we don't want it to be.
Or do you intend to do something else than what I understood?
Co-authored-by: Sarthak Kapoor <[email protected]>
|
Thanks for the fix, it looks better. For the truth of the statement I intend that update.m_def will always be a parent of section.m_def, not the opposite. So if Ah wait, in your suggestion this is a |
|
Ah, |
|
Okay, now I understand what you intend to do. I wonder if there are cases when this would fail. An extreme case will be that |
Co-authored-by: Sarthak Kapoor <[email protected]>
Is no issues, right? |
|
Yes, the |
|
Yes, as soon as I have time I'll dive into writing a test and thinking to edge cases |
which are the old tests not running @hampusnasstrom? I see 3.9-3.10-3.11 passing |
@aalbino2 this was a comment on your original PR which did not pass the tests. Now you seem to have fixed it with Sarthak's suggestions. |
That's it.
Instead of going for the m_section_def, one can go for the name of the property, that remains always the same when some child class is used.
What do you think?
Summary by Sourcery
Enhancements: