DOC: update validate_docstrings.py Validation script checks for tabs #20112
Conversation
scripts/validate_docstrings.py
Outdated
| @@ -455,6 +455,10 @@ def validate_one(func_name): | |||
| if not rel_desc: | |||
| errs.append('Missing description for ' | |||
| 'See Also "{}" reference'.format(rel_name)) | |||
|
|
|||
| if "\t" in doc.raw_doc: | |||
There was a problem hiding this comment.
Can you make this check for tabs only at the start of lines? We may have cases where we include the literal \t in the docstring (e.g. read_csv).
|
Hello @mariocj89! Thanks for updating the PR. Cheers ! There are no PEP8 issues in this Pull Request. 🍻 Comment last updated on March 10, 2018 at 12:40 Hours UTC |
scripts/validate_docstrings.py
Outdated
| @@ -455,6 +455,12 @@ def validate_one(func_name): | |||
| if not rel_desc: | |||
| errs.append('Missing description for ' | |||
| 'See Also "{}" reference'.format(rel_name)) | |||
|
|
|||
| for line in doc.raw_doc.splitlines(): | |||
| if re.match(" *\t", line): | |||
There was a problem hiding this comment.
I think the regex would be
if re.match(r"^\t", line)
There was a problem hiding this comment.
Indeed, missing the start of the line. Sorry about that. I was checking as well there is no spaces followed by tabs. Is it something we should check or just a plain "starts with a tab"
There was a problem hiding this comment.
Ahh, I see. So maybe ^ *\t? That's lines starting with 0 or more spaces followed by a tab.
There was a problem hiding this comment.
Do you want me to add a test file for this script? I can create a tests folder with a test for this script that will 1) prevent future mistakes and would have made easier to review a PR like this one. What do you think?
There was a problem hiding this comment.
Actually, There is already another PR adding the test file :)
Add a check to validate the docstrings dont have tabs at the begining of the line.
|
@datapythonista @jorisvandenbossche if you could glance over this. It looks good to me. |
| @@ -455,6 +455,12 @@ def validate_one(func_name): | |||
| if not rel_desc: | |||
| errs.append('Missing description for ' | |||
| 'See Also "{}" reference'.format(rel_name)) | |||
|
|
|||
| for line in doc.raw_doc.splitlines(): | |||
| if re.match("^ *\t", line): | |||
There was a problem hiding this comment.
'\t' in line is good enough, yes? we don't all any tabs
There was a problem hiding this comment.
If you see the comments above @TomAugspurger requested "Starts with tab" as there might be tabs to format a table or the like I think
There was a problem hiding this comment.
ok, not sure this is actually strict enough. as I said we don't allow any tabs
There was a problem hiding this comment.
I'm happy to change it back to allow no tabs at all. Let me know what you think @TomAugspurger
There was a problem hiding this comment.
@jreback There are cases where a \t is included in the text as literal character (for the explanation).
So I think only checking the start of a line is good.
There was a problem hiding this comment.
Unrelated to where \t is included, we could use re.search(r'^ *\t', self.raw_doc, flags=re.MULTILINE) and avoid the loop
There was a problem hiding this comment.
ups, sorry, I see now that you want to report the lines with the tabs...
|
@mariocj89 thanks! |
|
Thank you! |
Add a check to validate the docstrings don't have tabs.
The documentation uses whitespace only, adding the check will prevent
tabs being added in the sprint or future submissions
Sample output (elided non changed parts)