Skip to content

Conversation

@jbrockmendel
Copy link
Member

Preliminary to the PR that fixes setitem_with_indexer (#22036, #15686)

@jbrockmendel jbrockmendel added Indexing Related to indexing on series/frames, not to indexes themselves Internals Related to non-user accessible pandas implementation labels Feb 29, 2020
unfit_count = len(unfit_mgr_locs)

new_blocks = []
new_blocks: List[Block] = []
Copy link
Member

Choose a reason for hiding this comment

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

rather than add the variable annotation, does new_blocks need to be initialized outside the if else? make_block has the correct return type and the extend wouldn't be needed. (but I guess this mypy fixup is orthogonal to this PR)

Copy link
Member

@simonjayhawkins simonjayhawkins Mar 3, 2020

Choose a reason for hiding this comment

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

I'm not a fan of variable annotations when not required. but not a blocker.

def set(self, item: Label, value):
"""
Set new item in-place. Does not consolidate. Adds new Block if not
contained in the current set of items
Copy link
Member

Choose a reason for hiding this comment

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

minor nit. can you aim for one-liners for PEP 257compliance

@jreback jreback added this to the 1.1 milestone Mar 3, 2020
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

lgtm. @simonjayhawkins comments & merge on green.

@simonjayhawkins simonjayhawkins merged commit ce7670c into pandas-dev:master Mar 3, 2020
@simonjayhawkins
Copy link
Member

Thanks @jbrockmendel

@jbrockmendel jbrockmendel deleted the iset branch March 3, 2020 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Indexing Related to indexing on series/frames, not to indexes themselves Internals Related to non-user accessible pandas implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants