Skip to content

/pandas

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: Fix validation error type `RT01` and check in CI (#25356) #26234

Open
wants to merge 18 commits into
base: master
from
+453 −13

Conversation

Reviewers

@WillAyd WillAyd

@datapythonista datapythonista

Assignees
No one assigned
Labels
Docs Style
Projects
None yet
Milestone
  0.25.0
4 participants
@ihsansecer @WillAyd @datapythonista @jreback
@ihsansecer
Copy link
Contributor

commented Apr 28, 2019

  • closes #25356
  • passes ./scripts/validate_docstrings.py --errors=RT01
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff

@jreback jreback added the Style label Apr 28, 2019

@codecov

This comment has been minimized.

Sign in to view
Copy link

commented Apr 28, 2019

Codecov Report

Merging #26234 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #26234      +/-   ##
==========================================
- Coverage   91.98%   91.97%   -0.01%     
==========================================
  Files         175      175              
  Lines       52372    52373       +1     
==========================================
- Hits        48172    48168       -4     
- Misses       4200     4205       +5
Flag Coverage Δ
#multiple 90.52% <100%> (ø) ⬆️
#single 40.7% <100%> (-0.14%) ⬇️
Impacted Files Coverage Δ
pandas/core/indexes/multi.py 95.62% <ø> (ø) ⬆️
pandas/core/dtypes/dtypes.py 96.42% <ø> (ø) ⬆️
pandas/core/base.py 98.2% <ø> (ø) ⬆️
pandas/util/testing.py 90.61% <ø> (-0.11%) ⬇️
pandas/core/resample.py 97.22% <ø> (ø) ⬆️
pandas/plotting/_core.py 83.76% <ø> (ø) ⬆️
pandas/core/arrays/categorical.py 95.96% <ø> (ø) ⬆️
pandas/tseries/offsets.py 96.69% <ø> (ø) ⬆️
pandas/plotting/_misc.py 38.46% <ø> (ø) ⬆️
pandas/core/arrays/interval.py 93.06% <ø> (ø) ⬆️
... and 17 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 48ea04f...5920ad2. Read the comment docs.
@codecov

This comment has been minimized.

Sign in to view
Copy link

commented Apr 28, 2019 •
edited

Codecov Report

Merging #26234 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #26234      +/-   ##
==========================================
- Coverage   91.98%   91.97%   -0.01%     
==========================================
  Files         175      175              
  Lines       52372    52387      +15     
==========================================
+ Hits        48172    48182      +10     
- Misses       4200     4205       +5
Flag Coverage Δ
#multiple 90.52% <100%> (ø) ⬆️
#single 40.72% <62.5%> (-0.12%) ⬇️
Impacted Files Coverage Δ
pandas/core/indexes/multi.py 95.62% <ø> (ø) ⬆️
pandas/core/dtypes/dtypes.py 96.42% <ø> (ø) ⬆️
pandas/core/base.py 97.98% <ø> (-0.22%) ⬇️
pandas/util/testing.py 90.71% <ø> (ø) ⬆️
pandas/core/resample.py 97.22% <ø> (ø) ⬆️
pandas/plotting/_core.py 83.63% <ø> (-0.14%) ⬇️
pandas/core/arrays/categorical.py 95.96% <ø> (ø) ⬆️
pandas/tseries/offsets.py 96.69% <ø> (ø) ⬆️
pandas/plotting/_misc.py 38.46% <ø> (ø) ⬆️
pandas/core/arrays/interval.py 93.06% <ø> (ø) ⬆️
... and 36 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 48ea04f...d5f80f7. Read the comment docs.
@WillAyd

WillAyd requested changes Apr 29, 2019
View changes

Show resolved Hide resolved pandas/core/base.py Outdated
pandas/core/frame.py
@@ -1388,6 +1388,10 @@ def to_gbq(self, destination_table, project_id=None, chunksize=None,
or string contents. This is useful for remote server
authentication (eg. Jupyter/IPython notebook on remote host).
Returns
-------
None

This comment has been minimized.

Sign in to view
Copy link
@WillAyd

WillAyd Apr 29, 2019

Member

IIRC the validator should detect when there is no return and not require it to be explicitly stated

This comment has been minimized.

Sign in to view
Copy link
@ihsansecer

ihsansecer Apr 29, 2019 •
edited

Author Contributor

There is actually an empty return statement (This is the function returned by pandas to_gbq function) .

This comment has been minimized.

Sign in to view
Copy link
@WillAyd

WillAyd Apr 29, 2019

Member

Right but it still shouldn't be required (see #25008) - was this giving you an error?

This comment has been minimized.

Sign in to view
Copy link
@ihsansecer

ihsansecer Apr 29, 2019 •
edited

Author Contributor

Yes, it gives an error on my local. Should I check if it passes CI tests?

This comment has been minimized.

Sign in to view
Copy link
@WillAyd

WillAyd Apr 29, 2019

Member

Oh OK I see. Is this any different from the other io methods? This usually is None or str though totally familiar with pandas_gbq internals.

If it never actually returns anything probably better to update the code to not return instead of adding cruft to the docstring

This comment has been minimized.

Sign in to view
Copy link
@ihsansecer

ihsansecer Apr 29, 2019

Author Contributor

So I will remove the return sections of docstrings for to_gbq, to_hdf and to_pickle in this branch. Then open another PR for removing return statements of these functions. Right?

This comment has been minimized.

Sign in to view
Copy link
@WillAyd

WillAyd Apr 29, 2019

Member

Probably best to open the other PR before continuing here

This comment has been minimized.

Sign in to view
Copy link
@ihsansecer

ihsansecer Apr 29, 2019

Author Contributor

Right thanks

This comment has been minimized.

Sign in to view
Copy link
@datapythonista

datapythonista May 1, 2019

Member

I think detecting all the docstrings with Returns set to None is trivial. Personally I'm happy to leave this as it is until we fix the code that detects whether something can be detected (so we can validate RT01 in the CI), and we can easily detect and remove them later.

This comment has been minimized.

Sign in to view
Copy link
@ihsansecer

ihsansecer May 1, 2019

Author Contributor

I guess this issue will be resolved after merging #25462. None returning io functions are already handled in #26238. I think I can remove None since there will be no RT01 type errors after merge.
Show resolved Hide resolved pandas/core/frame.py
Show resolved Hide resolved pandas/core/generic.py Outdated
Show resolved Hide resolved pandas/core/groupby/generic.py Outdated
Show resolved Hide resolved pandas/core/groupby/groupby.py Outdated
Show resolved Hide resolved pandas/tseries/offsets.py Outdated

@WillAyd WillAyd added the Docs label Apr 29, 2019

@jreback jreback added this to the 0.25.0 milestone Apr 29, 2019

ihsansecer added a commit to ihsansecer/pandas that referenced this pull request Apr 29, 2019

@ihsansecer
CLN: Remove unnecessary io function returns 
These functions return a function either with empty return statement or doesn’t return. It causes docstring validation error with type RT01 (See pandas-dev#26234).
45b5409

@ihsansecer ihsansecer referenced this pull request Apr 29, 2019

Merged

CLN: Remove unnecessary io function returns #26238

1 of 1 task complete

@ihsansecer ihsansecer changed the title Fix validation error type `RT01` and check in CI DOC: Fix validation error type `RT01` and check in CI (#25356) Apr 29, 2019

@WillAyd WillAyd referenced this pull request Apr 30, 2019

Open

Remove return values for asserts #25462

4 of 4 tasks complete
@datapythonista

datapythonista reviewed May 1, 2019
View changes

Copy link
Member

left a comment

Good stuff, thanks for the work on this.

Added couple of comments, but looks good.
pandas/core/frame.py
@@ -1388,6 +1388,10 @@ def to_gbq(self, destination_table, project_id=None, chunksize=None,
or string contents. This is useful for remote server
authentication (eg. Jupyter/IPython notebook on remote host).
Returns
-------
None

This comment has been minimized.

Sign in to view
Copy link
@datapythonista

datapythonista May 1, 2019

Member

I think detecting all the docstrings with Returns set to None is trivial. Personally I'm happy to leave this as it is until we fix the code that detects whether something can be detected (so we can validate RT01 in the CI), and we can easily detect and remove them later.
pandas/core/generic.py Outdated
@@ -1845,7 +1850,13 @@ def __hash__(self):
' hashed'.format(self.__class__.__name__))

def __iter__(self):
"""Iterate over info axis"""
"""Iterate over info axis.

This comment has been minimized.

Sign in to view
Copy link
@datapythonista

datapythonista May 1, 2019

Member

Can you move the short summary to the next line please:

"""
Iterate over info axis.
pandas/core/generic.py
Returns
-------
None or str

This comment has been minimized.

Sign in to view
Copy link
@datapythonista

datapythonista May 1, 2019

Member

Just a preference, but I'd prefer str or None

This comment has been minimized.

Sign in to view
Copy link
@ihsansecer

ihsansecer May 1, 2019

Author Contributor

It is written in the same way as is in other to_* io functions. It returns str only when a path or buffer-like is not provided. I believe it mostly returns None which makes writing None or str more reasonable IMO.
Show resolved Hide resolved pandas/core/groupby/generic.py Outdated
pandas/core/groupby/groupby.py Outdated
Returns
-------
Series or DataFrame
Nth value within each group.

This comment has been minimized.

Sign in to view
Copy link
@datapythonista

datapythonista May 1, 2019

Member

Would N-th be correct and clearer?
pandas/core/indexes/base.py Outdated
Returns
-------
scalar
A value in the series with the indice of the key value in self.

This comment has been minimized.

Sign in to view
Copy link
@datapythonista

datapythonista May 1, 2019

Member

s/indice/index

probably better to capitalize Series too
Show resolved Hide resolved pandas/core/indexes/base.py Outdated

datapythonista and others added some commits May 1, 2019

@datapythonista @ihsansecer
DOC: Fix typo #26234 
Co-Authored-By: ihsansecer <abdullahsecer@std.sehir.edu.tr>
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
bd21eea
@ihsansecer
DOC: Make minor style changes (#26234)
Loading status checks…
d5f80f7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.