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

Unify CLIs #537

Merged
merged 11 commits into from
Apr 30, 2024
Merged

Unify CLIs #537

merged 11 commits into from
Apr 30, 2024

Conversation

stefmolin
Copy link
Contributor

@stefmolin stefmolin commented Mar 30, 2024

Changes:

  • Created numpydoc entry point (replacing the hook's entry point).
  • Moved CLI logic out of __main__.py and into a dedicated cli.py file.
  • Refactored the CLI to use subparsers and treat each of the unique functionalities as a subcommand like git: render, validate, and lint.
  • Updated test_main.py for changes and addition of lint option.
  • Removed the -m option from the hook, so that usage is now from numpydoc lint.
  • Updated the entry point for the pre-commit hook to numpydoc lint.
  • Updated validation docs.
  • Updated the test.yml workflow, but I didn't add a corresponding lint command as I don't fully understand what that part does.

Closes #467

Copy link
Member

@jarrodmillman jarrodmillman left a comment

Choose a reason for hiding this comment

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

Thanks!!

@jarrodmillman
Copy link
Member

Stefan is back and will take a look at this soon. He is a little backlogged. Thanks for your patience.

@stefanv
Copy link
Contributor

stefanv commented Apr 26, 2024

Thank you very much @stefmolin.

I see that I get a traceback when no import path is specified.

I think I suggested the numpydoc being an alias for numpydoc render before (bad Stéfan!), and now realize it is rather confusing. Perhaps we should let numpydoc (without arguments) print help? Or, at minimum, print a proper error message when import_path is None.

Copy link
Contributor

@stefanv stefanv left a comment

Choose a reason for hiding this comment

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

Looks good, I think the lint subparser perhaps needs to be hooked up more cleanly, but tested everything locally and it works. Thank you again!

numpydoc/__main__.py Show resolved Hide resolved
numpydoc/cli.py Outdated Show resolved Hide resolved
numpydoc/cli.py Outdated

validate = subparsers.add_parser(
"validate",
description="Validate the docstring with numpydoc.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we clarify here the distinction between validate and lint. They do the same, except the one operates on a function and the other on a file? But validate doesn't allow disabling certain rules, which confused me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it works for non-functions as well. How about this?

Validate an object's docstring conforms to the numpydoc standard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what to do about the second part of your comment. If you run numpydoc validate --help you see what can be passed as arguments, and there aren't any for disabling rules.

Copy link
Contributor

@stefanv stefanv Apr 29, 2024

Choose a reason for hiding this comment

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

"Validate that an object's docstring conforms to the numpydoc standard" (added "that")

Copy link
Contributor

Choose a reason for hiding this comment

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

Or, "Determine (or check?) whether an object's docstring..."

Copy link
Contributor

Choose a reason for hiding this comment

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

Otherwise just: "Validate an object's docstring against the numpydoc standard"

Copy link
Contributor

Choose a reason for hiding this comment

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

I may prefer the last version, but anything gramatically sound is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what to do about the second part of your comment. If you run numpydoc validate --help you see what can be passed as arguments, and there aren't any for disabling rules.

This was more of a question: should there be the option of disabling certain rules? Why are these not exactly equivalent in their functionality?

Copy link
Contributor Author

@stefmolin stefmolin Apr 29, 2024

Choose a reason for hiding this comment

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

I'm not sure what to do about the second part of your comment. If you run numpydoc validate --help you see what can be passed as arguments, and there aren't any for disabling rules.

This was more of a question: should there be the option of disabling certain rules? Why are these not exactly equivalent in their functionality?

While I agree this should eventually be the case, this definitely seems out of scope for this PR. At least the separate subcommands will not draw attention to arguments that have no effect like before (see #459).

numpydoc/cli.py Outdated Show resolved Hide resolved
numpydoc/hooks/validate_docstrings.py Outdated Show resolved Hide resolved
Copy link
Contributor

@stefanv stefanv left a comment

Choose a reason for hiding this comment

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

Thanks @stefmolin! The new interface is great.

@stefanv
Copy link
Contributor

stefanv commented Apr 29, 2024

@rossbar @jarrodmillman handing it over to you.

@jarrodmillman jarrodmillman merged commit e7c6baf into numpy:main Apr 30, 2024
23 checks passed
@stefanv stefanv added this to the 1.8.0 milestone Apr 30, 2024
@stefmolin stefmolin deleted the cli-alignment branch April 30, 2024 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unify cli validation interfaces
3 participants