ENH: Add ignore_index to sort_index #30578
Conversation
| sr = Series(original_list) | ||
| expected = Series(sorted_list, index=output_index) | ||
|
|
||
| # Test when inplace is False |
There was a problem hiding this comment.
can inplace be parameterised easily
There was a problem hiding this comment.
emm, i think in this case, if using parameterization, might not look very clean and less readable
There was a problem hiding this comment.
I think this should be parametrized - what do you think makes it less readable?
There was a problem hiding this comment.
ok with trying parameterize as a followup, as for .sort_values and .duplicated these tests look very much like this.
There was a problem hiding this comment.
emm, okay! i thought with inplace being Ture, we do not need to do something like df = df.sort_index(inplace=True), but False needs to be like this, and testing object will be different, one is testing if original changes, another is to test if assigned new df is changed.
Sorry to bring up discussion here, it definitely can be parametrized, was just my preference on such case. I will parametrize all three methods in the follow-up PR for sure!
simonjayhawkins
added
API Design
Enhancement
Reshaping
| ({"A": [1, 2, 3]}, {"A": [3, 2, 1]}, False, True, [0, 1, 2]), | ||
| ({"A": [1, 2, 3]}, {"A": [1, 2, 3]}, True, True, [0, 1, 2]), | ||
| ({"A": [1, 2, 3]}, {"A": [3, 2, 1]}, False, False, [2, 1, 0]), | ||
| ({"A": [1, 2, 3]}, {"A": [1, 2, 3]}, True, False, [0, 1, 2]), |
There was a problem hiding this comment.
Minor but can you use a non-default index as part of the expectation? Right now the parametrization can't disambiguate between the default index and ignore_index argument
| sr = Series(original_list) | ||
| expected = Series(sorted_list, index=output_index) | ||
|
|
||
| # Test when inplace is False |
There was a problem hiding this comment.
I think this should be parametrized - what do you think makes it less readable?
| sr = Series(original_list) | ||
| expected = Series(sorted_list, index=output_index) | ||
|
|
||
| # Test when inplace is False |
There was a problem hiding this comment.
ok with trying parameterize as a followup, as for .sort_values and .duplicated these tests look very much like this.
|
Lgtm |
|
thanks! will do @jreback |
|
Hi, |
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diff@jreback
I just looked back at #30114 and found out that i overlooked to add
ignore_indextosort_index, and this might be the reason this issue is still left open. I added this tosort_indexto close this issue.