-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
DOC: Fix PEP-8 issues in indexing-, missing_data-, options- and release.rst #24089
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
Conversation
Signed-off-by: Fabian Haase <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #24089 +/- ##
===========================================
- Coverage 92.21% 42.52% -49.69%
===========================================
Files 161 161
Lines 51684 51697 +13
===========================================
- Hits 47658 21982 -25676
- Misses 4026 29715 +25689
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #24089 +/- ##
=======================================
Coverage 92.21% 92.21%
=======================================
Files 162 162
Lines 51763 51763
=======================================
Hits 47733 47733
Misses 4030 4030
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, added few comments.
The fixes to the header will conflict with #24086, which is likely to be merged first.
doc/source/indexing.rst
Outdated
old_index = index | ||
del index | ||
old_index = index # noqa: F821 | ||
del index # noqa: F821 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this has :suppress:
and it's not visible, and it's broken to need the F821
, I guess we can simply get rid of it. Makes sense?
doc/source/indexing.rst
Outdated
@@ -1307,7 +1312,7 @@ The ``in`` and ``not in`` operators | |||
|
|||
try: | |||
old_d = d | |||
del d | |||
del d # noqa: F821 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as before, and I'm not sure it makes sense that d
is not defined here to need the noqa
, but it is in the previous line.
doc/source/indexing.rst
Outdated
@@ -1509,7 +1516,7 @@ default value. | |||
|
|||
.. ipython:: python | |||
|
|||
s = pd.Series([1,2,3], index=['a','b','c']) | |||
s = pd.Series([1, 2, 3], index=['a', 'b', 'c']) | |||
s.get('a') # equivalent to s['a'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mind leaving just two spaces before the comment, as in most places
doc/source/indexing.rst
Outdated
@@ -1829,7 +1838,7 @@ that you've done this: | |||
def do_something(df): | |||
foo = df[['bar', 'baz']] # Is foo a view? A copy? Nobody knows! | |||
# ... many lines here ... | |||
foo['quux'] = value # We don't know whether this will modify df or not! | |||
foo['quux'] = value # We don't know whether this will modify df or not! # noqa: E501, F821 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd move the comment to the previous line, and avoid E501
@FHaase can you address the review comments and fix the conflicts please? |
# Conflicts: # doc/source/indexing.rst # doc/source/missing_data.rst # doc/source/options.rst # doc/source/release.rst # setup.cfg
Signed-off-by: Fabian Haase <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, couple of comments
doc/source/options.rst
Outdated
|
||
.. ipython:: python | ||
:suppress: | ||
:okwarning: | ||
|
||
pd.reset_option('^display\.') | ||
pd.reset_option(r'^display.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want to delete the backslash, I guess this this string is used in a regular expression
doc/source/indexing.rst
Outdated
@@ -1785,17 +1771,17 @@ interpreter executes this code: | |||
|
|||
.. code-block:: python | |||
|
|||
dfmi.loc[:, ('one', 'second')] = value | |||
dfmi.loc[:, ('one', 'second')] = value # noqa: F821 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think of adding value = 'z'
before this, so we don't need to have all the F821
? Can be in a :suppress:
block, or directly here, whatever you think it's better.
:suppress: | ||
|
||
old_index = index | ||
del index |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mind rendering the page and making sure the result is not changed by removing these? can't really understand why these blocks are needed, but may be I'm missing something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the :suppress:
which resulted in
In [211]: old_index = index
---------------------------------------------------------------------------
NameError Traceback (most recent call last)
<ipython-input-211-27ea1dd9dc9d> in <module>
----> 1 old_index = index
NameError: name 'index' is not defined
In [212]: del index
---------------------------------------------------------------------------
NameError Traceback (most recent call last)
<ipython-input-212-b747d883ab88> in <module>
----> 1 del index
NameError: name 'index' is not defined
Same for the other (If the try block is removed as well). I doubt it makes any difference.
I noticed a df.index
in the block above so I tried to replace it with that, but del
is not defined.
Signed-off-by: Fabian Haase <[email protected]>
Signed-off-by: Fabian Haase <[email protected]>
…ssues # Conflicts: # doc/source/indexing.rst
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks @FHaase
can you merge master |
thanks @FHaase pls check the rendered docs to make sure all is ok in indexing.rst when they r built. |
* upstream/master: DOC: Fix PEP-8 issues in indexing-, missing_data-, options- and release.rst (pandas-dev#24089)
No description provided.