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: Improvement docstring of DataFrame.rank() #25328

Merged
merged 4 commits into from
May 6, 2019

Conversation

EdAbati
Copy link
Contributor

@EdAbati EdAbati commented Feb 15, 2019

  • passes git diff upstream/master -u -- "*.py" | flake8 --diff

I have made the changes requested in the pull request #23263

This is the output of the docstring validation:

3 Errors found:
	Parameter "method" description should finish with "."
	Parameter "na_option" description should finish with "."
	The first line of the Returns section should contain only the type, unless multiple values are being returned

In the previous pull request I have been told that I could ignore these 3 errors.

Please let me know if I can further improve the docstring.

Thanks a lot

@codecov
Copy link

codecov bot commented Feb 15, 2019

Codecov Report

Merging #25328 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #25328   +/-   ##
=======================================
  Coverage   91.72%   91.72%           
=======================================
  Files         173      173           
  Lines       52831    52831           
=======================================
  Hits        48458    48458           
  Misses       4373     4373
Flag Coverage Δ
#multiple 90.28% <ø> (ø) ⬆️
#single 41.72% <ø> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/generic.py 94.16% <ø> (ø) ⬆️

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 4be995c...9fbe884. Read the comment docs.

@codecov
Copy link

codecov bot commented Feb 15, 2019

Codecov Report

Merging #25328 into master will increase coverage by 0.25%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #25328      +/-   ##
==========================================
+ Coverage   91.72%   91.98%   +0.26%     
==========================================
  Files         173      175       +2     
  Lines       52831    52374     -457     
==========================================
- Hits        48458    48175     -283     
+ Misses       4373     4199     -174
Flag Coverage Δ
#multiple 90.53% <100%> (+0.25%) ⬆️
#single 40.71% <12.5%> (-1.01%) ⬇️
Impacted Files Coverage Δ
pandas/core/generic.py 93.54% <100%> (-0.63%) ⬇️
pandas/core/panel.py 35.43% <0%> (-36.32%) ⬇️
pandas/io/clipboard/__init__.py 39.21% <0%> (-17.65%) ⬇️
pandas/util/_doctools.py 0% <0%> (-12.88%) ⬇️
pandas/io/clipboard/clipboards.py 18.98% <0%> (-11.61%) ⬇️
pandas/io/formats/console.py 78.12% <0%> (-8.15%) ⬇️
pandas/plotting/_compat.py 83.33% <0%> (-4.17%) ⬇️
pandas/core/config_init.py 96.96% <0%> (-2.28%) ⬇️
pandas/core/sparse/series.py 93.3% <0%> (-2.24%) ⬇️
pandas/core/indexing.py 90.53% <0%> (-1.74%) ⬇️
... and 135 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 4be995c...7de320e. 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.

Can you actually take care of the three errors this has remaining? We've made some good progress on cleaning errors up in general so a little more strict now than we had been previously

@WillAyd WillAyd added the Docs label Feb 15, 2019
@WillAyd WillAyd added this to the 0.25.0 milestone Feb 15, 2019
@jreback
Copy link
Contributor

jreback commented Feb 16, 2019

lgtm. @WillAyd over to you.

@WillAyd
Copy link
Member

WillAyd commented Feb 19, 2019

@EdAbati can you merge master and address comments above? Should be simple fixes

@EdAbati
Copy link
Contributor Author

EdAbati commented Feb 19, 2019

Hi @WillAyd ,

I have solved the docstring validation errors above.

Please let me know if I should make any changes.

Thanks

* min: lowest rank in the group.
* max: highest rank in the group.
* first: ranks assigned in order they appear in the array.
* dense: like 'min', but rank always increases by 1 between groups.
Copy link
Member

Choose a reason for hiding this comment

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

Instead of adding periods to the end of each bullet what if you just moved the bullets above the short description you've added? Very minor but adding a period at the end of a bullet point is a hack to get validation to pass so would prefer not to do that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @WillAyd ,

I have tried to move the bullets above the short description, but the docstring validation will then produce these errors:

2 Errors found:
	Parameter "method" description should start with a capital letter
	Parameter "na_option" description should start with a capital letter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just noticed that for example in the pandas.DataFrame.any docstring there is a period at the end of each bullet and the validation does not return any error.
(If I understood correctly, this docstring is built from _bool_doc.)

How do you think it is the best way to use bullet lists?
Would you prefer that I substitute the 2 bullet lists with a description instead?

Copy link
Member

Choose a reason for hiding this comment

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

Right the example you mentioned for .any is exactly what I'm trying to avoid because I think it would look strange if every bulleted list in our documentation ends with periods just to get a validation check to pass.

Bullets are fine just add a very short description at the end to get validation to pass. Alternately there is an issue open to address the period error showing up for bullets in #20298 which would welcome a PR for

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @WillAyd ,

I will submit a PR for the issue #25461

@WillAyd
Copy link
Member

WillAyd commented Mar 19, 2019

@EdAbati can you address previous comment and merge master?

@EdAbati
Copy link
Contributor Author

EdAbati commented Mar 19, 2019

Hi @WillAyd,

I could not find a nice description for the parameters of this method without using bullet points.
So I have tried to fix the validation script as highlighted in the issue #25461
I am about to raise a PR for this.

If the PR is accepted, I was planning to only remove the bullet points at the end of each bullet.

Edit : I have just created a PR #25786

@jreback
Copy link
Contributor

jreback commented Apr 20, 2019

can you merge master

@WillAyd
Copy link
Member

WillAyd commented May 2, 2019

@EdAbati can you merge master? Don't think we need to wait on your other PR to get this one through

@EdAbati
Copy link
Contributor Author

EdAbati commented May 6, 2019

Hi @WillAyd ,

Apologies for the delay, I have been away and very busy lately. I have just removed the bullet points in the 2 parameter lists.

I will soon update the other PR as well.

@WillAyd WillAyd merged commit 94c8c94 into pandas-dev:master May 6, 2019
@WillAyd
Copy link
Member

WillAyd commented May 6, 2019

@EdAbati sorry for having taken so long on my end. This is a nice update - great job for a first contribution!

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.

3 participants