-
Notifications
You must be signed in to change notification settings - Fork 480
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 checkdocs_ignored_modules to exclude certain modules from checkdocs #2377
Conversation
This generally seems fine to me. There might be some opportunity to bikeshed the API here.. Should it be But I also think that we could have this as is, and deprecate this approach if we think we have a better API. The behavior here seems to be very well-defined, so deprecation should be relatively easy. |
2a6d580
to
cf5b11d
Compare
Forgot about this PR! Rebased. I think on balance that this is probably fine as-is if you are able to accept it. It would be possible to filter at the symbol level, but that seems like a different thing in that it is not a small change to this PR to do that, although I suppose module and symbol filtering could be exposed together with some kind of common (tree-walking-callback) API. I suspect these other requirement may not materialize, so perhaps let's lean towards saying YAGNI? It might be fine and/or nicer to do module filtering and symbol filtering separately if and when such a need arises? I don't think it should be What should I do with the changelog? Do I start a new heading? |
How about Beyond that, I think it would only need a CHANGELOG entry. |
cf5b11d
to
8576e35
Compare
I was just looking for a feature like this, great to see a PR already! |
8576e35
to
2b9ea2d
Compare
Rebased, updated to checkdocs_ignored_modules, changelog entry added. Ready to merge? |
Yes, indeed! Thank you @frankier! |
Fixes #2233
I went with
missingdocs_ignore
only rather than trying to check the other way i.e. that they are not included in the docs. I think this is an edge case and not something people need help verifying in practice.It might be nice to have a more general API at some point. I did think about this again this a bit, but I have had the need (a few times) for what's in this PR so maybe let's go with a KISS approach for now?