Skip to content

BLD: validate_docstrings.py add ignore_known_fail argument #25617

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

Conversation

dcreekp
Copy link
Contributor

@dcreekp dcreekp commented Mar 9, 2019

a solution for the problem of inherited docstrings not passing docstring tests preventing errors to be added to the CI code_checks.sh

@dcreekp dcreekp changed the title Validate docstrings add ignore known fail argument BLD: validate_docstrings.py add ignore known fail argument Mar 9, 2019
@dcreekp dcreekp changed the title BLD: validate_docstrings.py add ignore known fail argument BLD: validate_docstrings.py add ignore_known_fail argument Mar 9, 2019
@@ -946,9 +977,14 @@ def header(title, width=80, char='#'):
action='store_true', help='if this flag is set, '
'deprecated objects are ignored when validating '
'all docstrings')
argparser.add_argument('--ignore_known_fail', default=False,
Copy link
Contributor

Choose a reason for hiding this comment

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

Thoughts on the default? I would expect that if you're whitelisting thees failures, you wouldn't need to also pass --ignore-known-fail. Not a big deal either way since this is just in CI.

Can you add a couple sentences to contributing_docstring.rst saying when and how to use this?

@jreback jreback added the Docs label Mar 9, 2019
@WillAyd
Copy link
Member

WillAyd commented Mar 9, 2019

Might have missed some conversations but not sure I am on board with this change. Wouldn't it be easier to just update those docstrings on our end?

@dcreekp
Copy link
Contributor Author

dcreekp commented Mar 11, 2019

@WillAyd
If I understand your proposal, I don't think it's easier for a couple of reasons. Firstly these docstrings inherited from the datetime library are not in the pandas library as a string, at least I can't grep them.
More importantly, editting them to adhere to GL01 and GL02 may seem trivial but editting them to adhere to the rest of the list of errors is not trivial and just strange. If we intend to eventually have the entire list of errors on validate_docstrings.py to be checked by the CI then it makes sense to be able to exempt inherited docstrings. For example, RT01 tests for a returns section in the docstring and then there are more tests about the returns section. (Introspection to check whether the docstring is inherited is another option that was considered but decided to be overkill).
This way presents its own problems, like having docstrings that are not very helpful and having to make sure not to add a docstring to the whitelist by mistake. Still, unless there is something I'm unaware of, it is a good way forward to get more docstring errors to be checked by CI.

@WillAyd
Copy link
Member

WillAyd commented Apr 2, 2019

Why don't we just exclude sphinx from generating doc pages for these attributes? Most of the docstrings for these don't really offer much value:

image

Seems simpler than adding complexity with this PR

@WillAyd
Copy link
Member

WillAyd commented Apr 29, 2019

Closing as stale though not sure this is the right approach anyway. If interested in continuing let's pick up conversation back in referenced issue

@WillAyd WillAyd closed this Apr 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DOC: make scripts/validate_docstrings.py GL01 GL02 be able to exclude a list of known docstrings to ignore
4 participants