Skip to content
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

Add Doxygen binary path validation #90

Merged
merged 2 commits into from
Apr 2, 2024
Merged

Conversation

JakubAndrysek
Copy link
Owner

No description provided.

@JakubAndrysek
Copy link
Owner Author

@zyphlar please, test it.

Thanks, Jacob

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Type: Enhancement

PR Summary: This pull request introduces validation for the Doxygen binary path in the mkdoxy/doxyrun.py module. It adds a check at the initialization of the main class to ensure the provided Doxygen binary path is valid, either by being a recognized command in the system's PATH or by being a valid, executable file path. Additionally, it defines a custom exception for handling invalid Doxygen binary path scenarios.

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:

  • Consider enhancing the error message provided when the Doxygen binary path is invalid to include more detailed troubleshooting steps or common misconfigurations.
  • Ensure that the check for 'doxygen' in the system's PATH is robust across different operating systems, including those with case-sensitive file systems.
  • Review the implementation of the path validation to ensure it covers all common scenarios and edge cases where Doxygen might be installed or accessible.

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? ✨

Share Sourcery

Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

@@ -40,6 +42,14 @@ def __init__(
@param tempDoxyFolder: (str) Temporary folder for Doxygen.
@param doxyCfgNew: (dict) New Doxygen config options that will be added to the default config (new options will overwrite default options)
""" # noqa: E501

if not self.is_doxygen_valid_path(doxygenBinPath):
raise DoxygenBinPathNotValid(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (llm): Raising an exception for an invalid Doxygen binary path is a good practice for early failure. However, consider providing a more detailed error message or suggestions for common misconfigurations to aid in troubleshooting.

@return: (bool) True if the Doxygen binary path is valid, False otherwise.
"""
# If the path is just 'doxygen', search for it in the system's PATH
if doxygen_bin_path.lower() == "doxygen":
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (llm): Checking for 'doxygen' in the system's PATH is a user-friendly feature. However, ensure that this check is case-insensitive on all operating systems, including those with case-sensitive file systems.

@JakubAndrysek JakubAndrysek merged commit 929b14e into main Apr 2, 2024
14 checks passed
@JakubAndrysek JakubAndrysek deleted the doxygen-path-check branch April 2, 2024 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"The system cannot find the file specified" when Doxygen isn't in the PATH
1 participant