Skip to content

Conversation

@phofl
Copy link
Member

@phofl phofl commented Nov 19, 2020

Implemented comments

cc @jbrockmendel

@phofl phofl added the Clean label Nov 19, 2020
assert ser.iloc[2] == 2

def test_setitem_iloc_pure_position_based(self):
def test_iloc_setitem_pure_position_based(self):
Copy link
Member

Choose a reason for hiding this comment

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

the comment about test naming applied to a few tests in the previous PR.

BTW i have a branch called "collect" in which i collect small edits that dont necessarily merit their own PRs, then periodically make a PR titled "assorted cleanups"

Copy link
Member Author

@phofl phofl Nov 19, 2020

Choose a reason for hiding this comment

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

Ah yeah you are right :)

When I started contributing someone told me to try avoiding cluttering things together in one pr, which belong to different prs, so I tried to split everything up as good as possible.

Will collect clean ups in the future :)

Copy link
Member

Choose a reason for hiding this comment

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

When I started contributing someone told me to try avoiding cluttering things together in one pr, which belong to different prs, so I tried to split everything up as good as possible.

In general this is good advice. For the "assorted cleanups" PRs its important that the changes be pretty purely cosmetic so that they are super-easy to review.

Will collect clean ups in the future :)

It works for me but not for everyone. You find what works for you.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the help. Will try to collect them in the future if this is doable

if index.is_unique:
new_indexer = index.get_indexer([new_index[-1]])
if (new_indexer != -1).any():
# We get only in this method with loc, so can hard code
Copy link
Member

Choose a reason for hiding this comment

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

"in this method" -> "here"

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed,

@jbrockmendel
Copy link
Member

small comments, ping on green

@phofl
Copy link
Member Author

phofl commented Nov 19, 2020

@jbrockmendel jbrockmendel merged commit 793b635 into pandas-dev:master Nov 19, 2020
@jbrockmendel
Copy link
Member

thanks @phofl

@phofl phofl deleted the cln_indexing branch November 19, 2020 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2 participants