Align behavior of pre-commit hook and Sphinx extension

[stefmolin]

Closes #466

Changes made:

• Extracted validation check selection logic from the Sphinx extension into a utility function and added test cases.
• Changed the pre-commit hook check selection from an ignore list introduced in Pre-commit hook for running numpydoc validation #454 to the same strategy used by numpydoc_validation_checks on the Sphinx side.
• Added global exclusion option to pre-commit hook mirroring numpydoc_validation_exclude.
• Deprecated the override_GL08 pre-commit option, which is covered by the global exclusion option.
• Expanded the override logic to work for any check.
• Ported inline ignore comments logic from pre-commit side over to Sphinx extension and --validate sides.
• Brought override logic over to Sphinx extension side by creating numpydoc_validation_overrides, which allows the same flexibility that the override_<code> options in the pre-commit hook provide. Included this option in the doc/install.rst page and added some test cases.
• Updated pre-commit docs for changes in behavior.
• Created separate section for inline ignore comments in the docs since their scope expanded.
• Added cross-references between validation options in doc/validation.rst.

[stefmolin]

I think I have an issue with the type hints I added – I will comment when this is ready for review since I can't mark it as a draft.

Edit: Actually seems to be related to upstream changes made while I was working on this.

[stefmolin]

Issue has been solved. Ready for review.

[stefanv]

@stefmolin Thanks for this refactor, it looks great!

I should have written out #466 more clearly, because I actually liked the way you had it implemented; it felt much more similar to how other tools like ruff select their rules.

Since you have now looked at this code quite a bit, I'd like to hear your opinion on how it should look, in an ideal world.

I still think we can align the two, but am hoping we can deprecate the "Sphinx way" for the "pre-commit way".

@rossbar I think you've tried to explain to me before why numpydoc is a bit weird in the way it specifies rules, so perhaps you also want to weigh in.

[stefanv]

I've always found the numpydoc convention confusing; I think the exclusion list you had before is more logical.

[stefmolin]

@stefanv - I probably should have double-checked with you as well. Personally, I have not used the Sphinx extension for validation, so I can't speak to that. For my original use case, I wrote the pre-commit hook we merged last week and set it up on my personal and work projects to ignore certain rules. I imagine that the rule set is unlikely to change much if at all in the future and that most use cases for the Sphinx side are not just specifying a list of rules to run, but rather to run all and ignore a select few; therefore, I would agree with you that an ignore list as I had initially with the pre-commit hook is much easier to work with and more logical. The only reason I mirrored the Sphinx extension approach was to avoid breaking other projects' documentation builds. If we are comfortable with that though, then I am more than happy to switch both to the ignore list approach.

[stefanv]

What do you think, @rossbar?

[rossbar]

I have no preference re: how the configuration is implemented (i.e. an "allowlist", an "ignorelist", both, etc.)

FWIW the sphinx configuration supports both; i.e. either specifying a subset as an allowlist, or a "run-everything-except-these" ignorelist.

I think you've tried to explain to me before why numpydoc is a bit weird in the way it specifies rules, so perhaps you also want to weigh in.

The thing that I don't like about the way the validation is currently structured is the validator runs all the checks no matter what, and the configuration only affects which are reported. IMO it'd be a major improvement to refactor the validation so that it is modular, and only the selected checks are actually run. This is way out of scope for this discussion though!

I imagine that the rule set is unlikely to change much if at all in the future

I'd tend to agree - in fact, I think many of the implemented rules are already overly-specific to the point of not being very helpful.

and that most use cases for the Sphinx side are not just specifying a list of rules to run, but rather to run all and ignore a select few.

IME in trying this out for large, existing doc sets (e.g. numpy, scipy, networkx, etc.) - this hasn't been the case. Most of the validation checks fire way too often and add a bunch of noise, to the point where there are only a few that are worth enabling. Automatic reformatting (a la linters) would help alleviate this a lot, but again that's for another feature discussion.

[stefmolin]

FWIW the sphinx configuration supports both; i.e. either specifying a subset as an allowlist, or a "run-everything-except-these" ignorelist.

With the changes currently in this PR, the pre-commit hook also supports both using the same logic.

IME in trying this out for large, existing doc sets (e.g. numpy, scipy, networkx, etc.) - this hasn't been the case. Most of the validation checks fire way too often and add a bunch of noise, to the point where there are only a few that are worth enabling. Automatic reformatting (a la linters) would help alleviate this a lot, but again that's for another feature discussion.

In that case, perhaps it makes sense to use the Sphinx configuration for the pre-commit hook since you can provide a few rules to use rather than having to specify many to ignore.

[jarrodmillman]

@larsoner @rossbar @stefanv I would like to get this merged and then make a v1.6.0rc1 release later today. There seems to be agreement that this is an improvement and we can improve things in a future release if we want to.

Thoughts?

[larsoner]

+1 from me! You'll need to override branch protection rules to merge since pre-commit.ci won't run on this PR

[stefanv]

Let's get this in, and work towards refactoring the checking to not do unnecessary work in the future. Thanks so much @stefmolin!

[larsoner]

@jarrodmillman looks like this broke MNE-Python:

https://app.circleci.com/pipelines/github/mne-tools/mne-python/20169/workflows/d927bab9-9954-49ce-9cb2-2298f58cd29b/jobs/57313

Probably should sort this out before releasing!

[larsoner]

... I think this needs a if doc.source_file_name is not None perhaps?

[stefanv]

Thanks for the heads-up!

[stefmolin]

How did the Sphinx validation behave before when the source_file_name is None? Do you still run the validation?

[larsoner]

Yes. We're generate some doc strings on the fly, I assume that's what's breaking this but not sure