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

Convert itkgroup Doxygen script to Python. #5157

Merged

Conversation

blowekamp
Copy link
Member

Relates to: #3654

PR Checklist

  • No API changes were made (or the changes have been approved)
  • No major design changes were made (or the changes have been approved)
  • Added test (or behavior not changed)
  • Updated API documentation (or API not changed)
  • Added license to new files (if any)
  • Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
  • Added ITK examples for all new major features (if any)

Refer to the ITK Software Guide for
further development details if necessary.

We do not process doxygen on VXL.
@github-actions github-actions bot added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots area:Python wrapping Python bindings for a class area:Documentation Issues affecting the Documentation module labels Jan 23, 2025
@blowekamp
Copy link
Member Author

blowekamp commented Jan 23, 2025

The conversion was mostly done with git Copilot. The the following command was used for comparison the prior version:

 for fn in $(find ./ -name \*.h); do diff <(python ~/src/ITK/Utilities/Doxygen/itkgroup.py $fn) <( perl ~/src/ITK/Utilities/Doxygen/itkgroup.pl $fn) || echo FAILED: $fn; done

Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

If this works the same as before, this is good! More people are fluent in Python than Perl.

Copy link
Member

@jhlegarreta jhlegarreta left a comment

Choose a reason for hiding this comment

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

Use argparse with a separate method to parse input arguments? This parser method could host the documentation for the file, or else, we could document the file before the first import e.g.

(....)
"""
Docstring.
"""

import argparse
(...)

def _build_arg_parser():

    p = argparse.ArgumentParser(
        description=__doc__,
        formatter_class=argparse.RawTextHelpFormatter
    )
    p.add_argument("filename", type=str, help="Input path.")

    return p

def main():

    parser = _build_arg_parser()
    args = parser.parse_args()
    (...)

if __name__ == "__main__":
    main()

Unless the ITK style is the one you adopted.

@blowekamp
Copy link
Member Author

Use argparse with a separate method to parse input arguments? This parser method could host the documentation for the file, or else, we could document the file before the first import e.g.

(....)
"""
Docstring.
"""

import argparse
(...)

def _build_arg_parser():

    p = argparse.ArgumentParser(
        description=__doc__,
        formatter_class=argparse.RawTextHelpFormatter
    )
    p.add_argument("filename", type=str, help="Input path.")

    return p

def main():

    parser = _build_arg_parser()
    args = parser.parse_args()
    (...)

if __name__ == "__main__":
    main()

Unless the ITK style is the one you adopted.

I'll update to use a proper Python DocString for the file.

This is an improvement to modernize the scripts in a utility function. I do not find the additional effort to refactor the arguments worth the needed time.

Copy link
Member

@jhlegarreta jhlegarreta left a comment

Choose a reason for hiding this comment

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

I believe it would be informative to add a commit body message; will let other maintainers to merge.

Thanks for doing this Brad !!

@hjmjohnson hjmjohnson merged commit 1090d31 into InsightSoftwareConsortium:master Jan 24, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Documentation Issues affecting the Documentation module area:Python wrapping Python bindings for a class type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants