Add basic support for diagrams and dot files generation #99
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @JakubAndrysek - 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: 3 issues found
- 🟢 Docstrings: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| def __init__(self, xml: Element, parser: XmlParser, kind: Kind): | ||
| self.xml = xml | ||
| self.parser = parser | ||
| self.kind = kind | ||
| self.refid = self.xml.attrib.get("id") if self.xml is not None else None |
There was a problem hiding this comment.
suggestion (code_refinement): Consider extracting common initialization logic to a base class.
Both InheritanceGraph and CollaborationGraph classes have similar init methods. Extracting this to a base class could reduce code duplication.
| def print_node_content(node: Node, level=0, max_depth=1): | ||
| if level > max_depth: | ||
| return "" # Stop recursion when max depth is exceeded | ||
|
|
||
| indent = "\n" + " " * (level * 4) + "- " # Indentation for better readability | ||
| # node_representation = f"{indent}Node at Level {level}: {pformat(vars(node))}\n" | ||
| node_representation = f"{indent}Node at Level {level}: {node.name}\n" | ||
| # print all attributes of the node | ||
| for key, value in vars(node).items(): | ||
| if key == "children": | ||
| continue | ||
| node_representation += f"{indent} {key}: {pformat(value)}\n" | ||
|
|
||
| # Assuming each node has a list or iterable of child nodes in an attribute like `children` | ||
| for child in getattr(node, "children", []): | ||
| node_representation += print_node_content(child, level + 1, max_depth) | ||
|
|
||
| return node_representation |
There was a problem hiding this comment.
suggestion (performance): Consider limiting the recursion depth for large node trees.
The print_node_content function recursively prints node content. For very large node trees, consider adding a mechanism to limit the recursion depth to prevent potential stack overflow.
| @@ -64,6 +64,17 @@ def save(self, path: str, output: str): | |||
| with open(os.path.join(self.tempDoxyDir, pathRel), "w", encoding="utf-8") as file: | |||
| file.write(output) | |||
|
|
|||
| def save_image(self, path: str, image_source_link: str): | |||
There was a problem hiding this comment.
issue (complexity): Consider refactoring the file copying logic in save_image to improve code maintainability and robustness.
The addition of the save_image method introduces some complexity and potential maintenance challenges. Here are a few suggestions to streamline this functionality:
-
Consider abstracting the file copying logic into a separate method, such as
copy_file, to reduce code duplication and improve readability. This method can ensure the destination directory exists and then perform the copy operation, encapsulating these steps away from the higher-level logic ofsave_image. -
The deeply nested code within
save_imagemakes it a bit harder to follow. Simplifying this by using the proposedcopy_filemethod could make the overall flow more understandable. -
It's important to handle potential errors that can occur during file operations, such as permissions issues or disk space limitations. While the current code doesn't explicitly address these, relying on built-in functions like
shutil.copywithin acopy_filemethod could offer more robustness by leveraging their internal error handling. -
The commented-out code and somewhat duplicated path construction logic could be cleaned up to further improve the maintainability of this code. Removing unused code and possibly centralizing path construction could be beneficial.
By addressing these points, the code would not only become cleaner and more maintainable but also potentially more robust against common file operation errors.
| @@ -94,10 +95,30 @@ | |||
| @param data (dict): Data to render the template. | |||
| @return (str): Rendered template. | |||
| """ | |||
|
|
|||
| def print_node_content(node: Node, level=0, max_depth=1): | |||
There was a problem hiding this comment.
issue (complexity): Consider simplifying the render method by maintaining the use of Jinja2 Environment and separating the print_node_content logic.
The refactoring introduces a more complex approach by removing the use of Environment from Jinja2 and directly creating Template instances. This change loses the benefits of a shared environment, such as centralized configuration and filter management. Additionally, the introduction of the print_node_content function within the render method significantly increases its complexity by adding recursive functionality directly inside it. This not only makes the method harder to understand but also violates the Single Responsibility Principle.
Moreover, the method now passes additional functions (pformat, vars, print_node_content) as variables to the template, increasing the complexity of the data managed within the template and the coupling between the Python code and the Jinja2 template.
A more maintainable approach would be to keep the use of an Environment for managing templates, which allows for a cleaner and more centralized configuration. Also, consider keeping template-related logic separate from data processing logic, such as the content inspection and formatting done by print_node_content. This function, if necessary, could be better placed outside the render method, possibly as a utility function or within the data model itself, to keep the rendering logic clean and focused solely on template rendering.
This approach would not only simplify the current implementation but also enhance the maintainability and readability of the code by clearly separating concerns and utilizing Jinja2 features more effectively.
| @@ -87,6 +87,9 @@ def __init__( | |||
| self._initializer = Property.Initializer(self._xml, parser, self._kind) | |||
| self._definition = Property.Definition(self._xml, parser, self._kind) | |||
| self._programlisting = Property.Programlisting(self._xml, parser, self._kind) | |||
| self._inheritancegraph = Property.InheritanceGraph(self._xml, parser, self._kind) | |||
There was a problem hiding this comment.
issue (complexity): Consider encapsulating the new graph-related functionality into a separate class.
The introduction of _inheritancegraph, _collaborationgraph, and the commented-out _directorydependency properties, along with their associated logic, significantly increases the complexity of this class. This expansion not only adds more responsibilities to the class but also enlarges its public interface with additional conditional checks and methods. It's generally beneficial to adhere to the Single Responsibility Principle to keep classes focused and maintainable.
Consider encapsulating the new graph-related functionality into a separate class. This would streamline the responsibilities of the original class, making it easier to understand and maintain, while also providing a dedicated space for managing graph properties and any future expansions in this area. Encapsulation would also address the issue of the commented-out code by providing a clear context for its inclusion or future development.
A separate GraphProperties class could manage the inheritance and collaboration graphs, and potentially the directory dependency, should it be reintroduced. This approach would simplify the original class's interface and reduce the cognitive load required to understand its behavior, leading to a cleaner, more modular design.
| @@ -11,7 +12,7 @@ | |||
|
|
|||
|
|
|||
| class Doxygen: | |||
| def __init__(self, index_path: str, parser: XmlParser, cache: Cache): | |||
| def __init__(self, index_path: PurePath, parser: XmlParser, cache: Cache): | |||
There was a problem hiding this comment.
issue (code-quality): Low code quality found in Doxygen.__init__ - 23% (low-code-quality)
Explanation
The quality score for this function is below the quality threshold of 25%.This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
|
Hey, what is the status of this change? I would really like to see it merged |
|
Hi, I would like to merge this feature during the summer holidays when I will have more time for projects like this. |
No description provided.