-
-
Notifications
You must be signed in to change notification settings - Fork 19
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
Update MkDoxy core #110
base: main
Are you sure you want to change the base?
Update MkDoxy core #110
Conversation
Reviewer's Guide by SourceryThis pull request rewrites the core of the MkDoxy plugin to improve configuration processing, mark outdated config options, and add new ones. It also introduces a migration tool to convert old config to the new format. The changes include refactoring classes and methods to use the new configuration structure, updating method names to follow snake_case convention, adding detailed docstrings, and updating tests and documentation accordingly. File-Level Changes
Tips
|
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.
Hey @JakubAndrysek - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Hard-coded Google Analytics property ID found. (link)
- Hard-coded Google Analytics property ID found. (link)
- Hard-coded Google Analytics property ID found. (link)
- Hard-coded Google Analytics property ID found. (link)
Here's what I looked at during the review
- 🟡 General issues: 9 issues found
- 🔴 Security: 4 blocking issues
- 🟡 Testing: 2 issues found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
self.doxy_config = process_configuration(self.config) | ||
|
||
def on_files(self, files: files.Files, config: base.Config) -> files.Files: | ||
log.info(f"Start plugin {plugin_name}") |
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.
suggestion: Configuration processing
The process_configuration
function is called to process the plugin configuration. Ensure that this function handles all necessary validation and transformation of the configuration data.
self.doxy_config = process_configuration(self.config) | |
def on_files(self, files: files.Files, config: base.Config) -> files.Files: | |
log.info(f"Start plugin {plugin_name}") | |
try: | |
self.doxy_config = process_configuration(self.config) | |
except Exception as e: | |
log.error(f"Configuration processing failed: {e}") | |
raise | |
log.info(f"Start plugin {plugin_name}") |
project_data: MkDoxyConfigProject | ||
log.info(f"-> Processing project '{project_name}'") |
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.
suggestion: Type hint for project_data
The project_data
variable is type hinted as MkDoxyConfigProject
. Ensure that this type hint is accurate and that MkDoxyConfigProject
is correctly defined.
project_data: MkDoxyConfigProject | |
log.info(f"-> Processing project '{project_name}'") | |
project_data = self.doxy_config.projects[project_name] | |
if not isinstance(project_data, MkDoxyConfigProject): | |
raise TypeError(f"Expected MkDoxyConfigProject, got {type(project_data).__name__}") | |
log.info(f"-> Processing project '{project_name}'") |
temp_doxy_folder.mkdir(parents=True, exist_ok=True) | ||
|
||
# Check src changes -> run Doxygen | ||
doxygenRun = DoxygenRun( | ||
self.config["doxygen-bin-path"], | ||
project_data.get("src-dirs"), | ||
tempDirApi, | ||
project_data.get("doxy-cfg", {}), | ||
project_data.get("doxy-cfg-file", ""), | ||
doxygen = DoxygenGenerator( |
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.
issue (bug_risk): Directory creation
The temp_doxy_folder
directory is created if it does not exist. Ensure that this operation is thread-safe if the plugin is used in a multi-threaded environment.
hash_file_path = Path.joinpath(self.temp_doxy_folder, self.hash_file_name) | ||
if hash_file_path.is_file(): |
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.
suggestion: Hash file path construction
The hash_file_path
is constructed using Path.joinpath
. Ensure that this method is used consistently throughout the code for path manipulations.
self.load_template_from_folder(path) | ||
|
||
# test if custom template directory exists | ||
if custom_template_dir: |
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.
suggestion: Loading templates from folder
The load_template_from_folder
method is called to load templates from the specified folder. Ensure that this method handles all edge cases and errors gracefully.
link: https://www.linkedin.com/in/jakub-andrysek/ | ||
analytics: | ||
provider: google | ||
property: G-8WHJ2N4SHC |
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.
🚨 issue (security): Hard-coded Google Analytics property ID found.
Consider using an environment variable to store the Google Analytics property ID to avoid exposing it in the source code.
link: https://www.linkedin.com/in/jakub-andrysek/ | ||
analytics: | ||
provider: google | ||
property: G-8WHJ2N4SHC |
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.
🚨 issue (security): Hard-coded Google Analytics property ID found.
Consider using an environment variable to store the Google Analytics property ID to avoid exposing it in the source code.
link: https://www.linkedin.com/in/jakub-andrysek/ | ||
analytics: | ||
provider: google | ||
property: G-8WHJ2N4SHC |
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.
🚨 issue (security): Hard-coded Google Analytics property ID found.
Consider using an environment variable to store the Google Analytics property ID to avoid exposing it in the source code.
mkdoxy/doxy_config.py
Outdated
@return: None | ||
@throws ConfigurationError: If the configuration is invalid. | ||
""" | ||
if len(legacy_options) > 0: |
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.
suggestion (code-quality): Simplify sequence length comparison (simplify-len-comparison
)
if len(legacy_options) > 0: | |
if legacy_options: |
@@ -12,9 +13,9 @@ | |||
|
|||
|
|||
class Doxygen: | |||
def __init__(self, index_path: str, parser: XmlParser, cache: Cache): | |||
def __init__(self, index_path: Path, parser: XmlParser, cache: Cache): |
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.
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.
9d74b55
to
d24072a
Compare
Rewrite the core of MkDoxy plugin
Summary by Sourcery
This pull request rewrites the core of the MkDoxy plugin, improving configuration processing, marking outdated config options, and adding new ones. It also introduces a migration tool to convert old configurations to the new format, enhances logging and error handling, and refactors the template loading mechanism.