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

DOC: Docstring script to not require ending period for bullet points #25786

Closed
wants to merge 26 commits into from

Conversation

EdAbati
Copy link
Contributor

@EdAbati EdAbati commented Mar 19, 2019

This PR tries to solve the issue #25461 .

I thought that the validator should check that the last line of the parameter description is a part of a bullet point before raising the error PR09.

In order to do that, I had to add an extra method in the Docstring class that return a parameter description line by line (in a list). This is because Docstring.parameter_desc() returns the description as 1 string.

Please let me know how I can improve this or if I did not consider something.

@codecov
Copy link

codecov bot commented Mar 19, 2019

Codecov Report

Merging #25786 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #25786      +/-   ##
==========================================
+ Coverage   91.26%   91.26%   +<.01%     
==========================================
  Files         172      172              
  Lines       52965    52965              
==========================================
+ Hits        48337    48338       +1     
+ Misses       4628     4627       -1
Flag Coverage Δ
#multiple 89.82% <ø> (ø) ⬆️
#single 41.74% <ø> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/util/testing.py 89.44% <0%> (+0.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c975fc4...9bb2fa4. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 19, 2019

Codecov Report

Merging #25786 into master will decrease coverage by 0.98%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #25786      +/-   ##
==========================================
- Coverage   93.02%   92.04%   -0.99%     
==========================================
  Files         182      180       -2     
  Lines       50257    50713     +456     
==========================================
- Hits        46750    46677      -73     
- Misses       3507     4036     +529
Flag Coverage Δ
#multiple 90.68% <ø> (-1.01%) ⬇️
#single 41.88% <ø> (-0.62%) ⬇️
Impacted Files Coverage Δ
pandas/io/gbq.py 88.88% <0%> (-11.12%) ⬇️
pandas/io/excel/_openpyxl.py 84.71% <0%> (-3.23%) ⬇️
pandas/util/_decorators.py 92.13% <0%> (-3.22%) ⬇️
pandas/core/dtypes/cast.py 90.72% <0%> (-2.34%) ⬇️
pandas/io/formats/printing.py 86.72% <0%> (-2.11%) ⬇️
pandas/core/indexing.py 93.48% <0%> (-1.55%) ⬇️
pandas/core/dtypes/common.py 96.35% <0%> (-1.44%) ⬇️
pandas/core/arrays/categorical.py 95.92% <0%> (-1.27%) ⬇️
pandas/core/groupby/ops.py 96% <0%> (-0.92%) ⬇️
pandas/core/internals/construction.py 95.95% <0%> (-0.84%) ⬇️
... and 163 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f6a5dd4...43ae6ea. Read the comment docs.

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

Please add test(s)

@WillAyd WillAyd added Docs CI Continuous Integration labels Mar 19, 2019
@pep8speaks
Copy link

pep8speaks commented Mar 24, 2019

Hello @EdAbati! 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 2019-10-24 22:25:43 UTC

@WillAyd
Copy link
Member

WillAyd commented Mar 26, 2019

@EdAbati as mentioned if you can add tests will take a look

@EdAbati
Copy link
Contributor Author

EdAbati commented Mar 26, 2019

Hi @WillAyd , sorry for the delay.

I just added a couple of tests.
Please let me know if and how I can improved this.

Thank you very much

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

Tests look good! I think the implementation can be simplified / made clearer

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

@EdAbati this is moving in the right direction but still needs to be simplified. Anything which isn't explicitly tested and unnecessary conversion of objects to different container types needs to be removed

@jreback
Copy link
Contributor

jreback commented May 12, 2019

can you merge master

@EdAbati
Copy link
Contributor Author

EdAbati commented Jun 18, 2019

Hi @WillAyd,
I was wondering if you had the chance to have a look at this,
Is there anything I should improve/change?

@WillAyd
Copy link
Member

WillAyd commented Jun 19, 2019 via email

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

Tests look great! Just want to keep simplifying the implementation a bit but almost there.

Thanks for sticking with this

@jbrockmendel
Copy link
Member

@EdAbati can you rebase

@TomAugspurger
Copy link
Contributor

@EdAbati CI should be fixed now if you can merge master again.

@WillAyd
Copy link
Member

WillAyd commented Aug 23, 2019

Hi @EdAbati I've been on vacation the past few weeks so haven't looked in detail. Back in a few days will review within the next week

@jreback
Copy link
Contributor

jreback commented Sep 8, 2019

what is status here?

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

Cool thanks - this is starting to look simpler. Right now I don't think 100% accurate but an improvement over where we are, so might just move forward with it and leave rough edges to a follow up.

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.

Sorry for the late review @EdAbati, didn't see this before. Can you fix the conflicts and have a look at the comments.

@datapythonista datapythonista changed the title TST: Fix Validation Script to not error out if ends with Bullet Points DOC: Docstring script to not require ending period for bullet points Oct 13, 2019
@EdAbati EdAbati closed this Oct 25, 2019
@datapythonista
Copy link
Member

The PR I mentioned has been merged already, you can open this PR in numpydoc now.

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.

Fix Validation Script to Not Error Out if Bullet Points Don't End with Period
7 participants