-
Notifications
You must be signed in to change notification settings - Fork 19
bugfix draw function for modules #344
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
bugfix draw function for modules #344
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run //src:license-checkStatus: Click to expand output |
|
The created documentation from the pull request is available at: docu-html |
| # includes_back could deliver multiple needs; onlyreturn Modules | ||
| parents = need.get("includes_back", []) | ||
| if isinstance(parents, str): | ||
| parents = [parents] |
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.
Will this not automatically be a list according to sphinx_needs?
So would this then make it a double list? [[parents]] if 'parents` itself already is a list?
Is this the wanted behaviour?
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.
Yes, this is unnecessary.
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.
Fixed and add warning.
MaximilianSoerenPollak
left a comment
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.
Seems okay from my side.
|
|
||
| if len(module) > 1: | ||
| logger.warning( | ||
| f"{component}: included in multiple modules: {module}. Returning first." |
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.
Can be fixed in future PR.
Is it somewhere documented why only the first one is returned?
Or what the logic behind it is?
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.
This is the implicit logic, that a component belongs only to one module. Module is something like a bazel module or a package. And the component should only be in one. Sure, it could be used or integrated in more then one, but the real implementation should only be in one. I'm not sure if there are any circumstances (base libraries ???) for an exception from this rule, therefore I give only the warning. But in general it is only one. If we want to modify the draw function to show also multiple modules, then I guess more have to be changed. But will discuss this with the colleagues.
📌 Description
Fix draw function for module in case multiple links are on module.
🚨 Impact Analysis
✅ Checklist