Skip to content

TST: Add test for setting item using python list (#19406) #37840

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.

Sign up for GitHub

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

Jump to bottom
Merged
MarcoGorelli merged 4 commits into from
Nov 15, 2020
Merged

TST: Add test for setting item using python list (#19406) #37840

MarcoGorelli merged 4 commits into from
Nov 15, 2020

Conversation

@paxcodes
Copy link
Contributor

@paxcodes paxcodes commented Nov 14, 2020

arw2019
arw2019 reviewed Nov 14, 2020
View reviewed changes
Copy link
Contributor

@arw2019 arw2019 left a comment

Choose a reason for hiding this comment

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

Thanks @paxcodes for the PR!

Some small comments, otherwise LGTM

ivanovmg
ivanovmg reviewed Nov 14, 2020
View reviewed changes
Copy link
Contributor

@ivanovmg ivanovmg left a comment

Choose a reason for hiding this comment

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

@paxcodes thank you for the PR!
Would you consider parametrizing the test with various null values?

phofl
phofl reviewed Nov 14, 2020
View reviewed changes
@paxcodes paxcodes requested a review from phofl November 14, 2020 20:02
@phofl
Copy link
Member

phofl commented Nov 14, 2020

One nitpick from my side: you coud parametrize somethink like func with [list, np.array, Series] and define mask in the test through mask=func([True, False, True]) Would make parametrization shorter

@paxcodes
Copy link
Contributor Author

paxcodes commented Nov 14, 2020

@phofl Ooh nice. Will do.

@paxcodes
Copy link
Contributor Author

paxcodes commented Nov 14, 2020

@phofl There. Much more concise. ✨

@phofl
Copy link
Member

phofl commented Nov 14, 2020

lgtm

ivanovmg
ivanovmg approved these changes Nov 15, 2020
View reviewed changes
Copy link
Contributor

@ivanovmg ivanovmg left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me.

@MarcoGorelli MarcoGorelli added this to the 1.2 milestone Nov 15, 2020
@MarcoGorelli MarcoGorelli added the Testing pandas testing functions or related to the test suite label Nov 15, 2020
@MarcoGorelli
Copy link
Member

MarcoGorelli commented Nov 15, 2020

Thanks @paxcodes , congrats on your first PR to pandas! Do let us know if you want pointers for other issues if you'd like to tackle them too

@MarcoGorelli MarcoGorelli merged commit 5b98dc6 into pandas-dev:master Nov 15, 2020
@paxcodes
Copy link
Contributor Author

paxcodes commented Nov 15, 2020

Thanks @MarcoGorelli and again, thanks for your time yesterday at the sprint!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@arw2019 arw2019 Awaiting requested review from arw2019

@phofl phofl Awaiting requested review from phofl

1 more reviewer

@ivanovmg ivanovmg ivanovmg approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Testing pandas testing functions or related to the test suite

Projects

None yet

Milestone

1.2

Development

Successfully merging this pull request may close these issues.

Boolean indexing and assignment from python list broken

5 participants

@paxcodes @phofl @MarcoGorelli @ivanovmg @arw2019