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

CI: Adding script to validate consistent and correct capitalization among headings in documentation (#26941) #31114

Merged
merged 63 commits into from
Mar 7, 2020

Conversation

tonywu1999
Copy link
Contributor

@tonywu1999 tonywu1999 commented Jan 17, 2020

Closes #26941

This script should be running correctly to validate capitalization for various rst files. Many keywords will need to be added into the script as more and more files are tested on this script. Currently testing on the following rst files through code_checks.sh:

doc/source/development/contributing.rst
doc/source/index.rst doc/source/ecosystem.rst

contributing.rst and ecosystem.rst should produce an exit code of 1, indicating there are incorrect capitalization in their headings.

@pep8speaks
Copy link

pep8speaks commented Jan 17, 2020

Hello @tonywu1999! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-03-07 15:09:27 UTC

@WillAyd
Copy link
Member

WillAyd commented Jan 17, 2020

This is a lot of code; is there no third party package to do something like this?

@tonywu1999
Copy link
Contributor Author

@WillAyd

I've looked around when trying to solve this problem and I don't think there's a third party package that does everything that we need. Ultimately, the more complicated parts of the script involve the combination of finding headers in an RST file & determining if a string follows our capitalization convention while ensuring keywords like pandas are properly capitalized too.

I will keep looking for third party packages online to hopefully simplify the lines of this script. In the meantime, my goal right now is to check if the script is working as expected.

@jbrockmendel
Copy link
Member

@tonywu1999 looks like some (maybe new?) docstrings dont pass this check. can you update

@datapythonista
Copy link
Member

@tonywu1999 can you fix the titles you left to see how the errors look like? I think we can get this merged once it's green.

@datapythonista
Copy link
Member

Also, please merge master into your branch, so we make sure we're validating the latest version of the docs. Thanks!

@tonywu1999
Copy link
Contributor Author

Hi @datapythonista

I fixed the titles just now and committed the changes. There should be title changes in doc/source/development/contributing.rst

Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

Thanks, can you have a look at the CI? I guess it's unrelated, but if you can merge master and make sure it's green, well be able to merge this.

@tonywu1999
Copy link
Contributor Author

@datapythonista
I'm not sure what you meant by that. I merged with master and CI/Checks, which my code affects, is running fine. Are you referring to the two X's for pandas-dev.pandas and pandas-dev.pandas(Linux py36_locale) ?

@datapythonista
Copy link
Member

Yes, something failed in that build. Probably something unrelated to this pr, but we need to have it green to merge this. I'm in my phone and can't look at the logs right now, but if you can have a look and see what's the problem, that would be great.

@tonywu1999
Copy link
Contributor Author

@datapythonista

I looked at the error logs. However, I'm not really sure what's causing the errors.

@datapythonista
Copy link
Member

I looked at the error logs. However, I'm not really sure what's causing the errors.

If you navigate all the way to the azure pipeline logs, and look for errors, you'll find this line:

AssertionError: Caused unexpected warning(s): [('ResourceWarning', ResourceWarning('unclosed <ssl.SSLSocket [closed] fd=16, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM,

Which looks like a systems error, that (hopefully) shouldn't repeat if the job is executed again. If you merge master into your branch, and push, the CI will run again, and hopefully everything will be green this time. And we'll be able to merge this with confidence.

@tonywu1999
Copy link
Contributor Author

@datapythonista

I think all checks have passed now.

Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

Thanks @tonywu1999, and sorry for the delay.

I added few minor final comments more if you don't mind, I think they'll improve readability and future maintenance.

scripts/validate_rst_title_capitalization.py Outdated Show resolved Hide resolved
scripts/validate_rst_title_capitalization.py Outdated Show resolved Hide resolved
scripts/validate_rst_title_capitalization.py Outdated Show resolved Hide resolved
scripts/validate_rst_title_capitalization.py Outdated Show resolved Hide resolved
scripts/validate_rst_title_capitalization.py Outdated Show resolved Hide resolved
scripts/validate_rst_title_capitalization.py Outdated Show resolved Hide resolved
scripts/validate_rst_title_capitalization.py Show resolved Hide resolved
scripts/validate_rst_title_capitalization.py Show resolved Hide resolved
scripts/validate_rst_title_capitalization.py Outdated Show resolved Hide resolved
scripts/validate_rst_title_capitalization.py Outdated Show resolved Hide resolved
Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

I see there are still some pending comments from the previous review, let me know when this is ready to be reviewed again.

scripts/validate_rst_title_capitalization.py Outdated Show resolved Hide resolved
scripts/validate_rst_title_capitalization.py Outdated Show resolved Hide resolved
@datapythonista datapythonista merged commit 2a2258d into pandas-dev:master Mar 7, 2020
@datapythonista
Copy link
Member

Thanks for this @tonywu1999, good improvement.

Can you please open an issue to fix the titles in the rest of the files?

@tonywu1999
Copy link
Contributor Author

@datapythonista

Thanks for your guidance! I opened a new issue (#32550) to address fixing the titles in the rest of the files

@simonjayhawkins simonjayhawkins added this to the 1.1 milestone Mar 9, 2020
SeeminSyed pushed a commit to CSCD01-team01/pandas that referenced this pull request Mar 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration Docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DOC: Validate consistency of title capitalization
7 participants