Skip to content
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

STYLE: Resolved Inconsistent namespace #40286

Merged
merged 22 commits into from
Mar 11, 2021
Merged

STYLE: Resolved Inconsistent namespace #40286

merged 22 commits into from
Mar 11, 2021

Conversation

deepang17
Copy link
Contributor

@deepang17 deepang17 commented Mar 7, 2021

@MarcoGorelli
Copy link
Member

Hi @deepang17 - instead of excluding a file, can we change test_unique_tuples so the last argument of the function is uniques instead of unique?

@deepang17
Copy link
Contributor Author

Sure, will do that.

@lithomas1 lithomas1 added the Code Style Code style, linting, code_checks label Mar 7, 2021
@MarcoGorelli
Copy link
Member

MarcoGorelli commented Mar 7, 2021

Thanks @deepang17 , I'll just mark this as draft pending on #39992 (comment) , will then reopen (depending on what we hear back, you may need to rebase / fix conflicts)

@MarcoGorelli MarcoGorelli added this to the 1.3 milestone Mar 7, 2021
@MarcoGorelli MarcoGorelli marked this pull request as draft March 7, 2021 15:17
@MarcoGorelli
Copy link
Member

Hi @deepang17 - #40310 is in, do you want to rebase and fix up any remaining errors so we can get this in?

@deepang17
Copy link
Contributor Author

Hello, @MarcoGorelli I have completed work from my side. If you think that changing from unique to uniques is not a breaking change then this works fine.

@MarcoGorelli MarcoGorelli marked this pull request as ready for review March 8, 2021 17:24
@MarcoGorelli
Copy link
Member

You'll also need to change it to uniques in

    @pytest.mark.parametrize(
        "arr, unique",

It's just a variable in a test, so it's not a breaking change

If you could fetch and merge upstream/master and fixup "unique" in pytest.mark.parametrize then this should be good to go

@deepang17
Copy link
Contributor Author

Ok, will do that.

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

Thanks for updating - there's still some files to be fixed up though, which are failing the pre-commit check

@deepang17
Copy link
Contributor Author

image

@MarcoGorelli these new files are producing the error.

image

@deepang17
Copy link
Contributor Author

Shall I convert them to arrays?

@MarcoGorelli
Copy link
Member

I'd suggest renaming to arr rather than arrays, as here there's only one of them (uniques in test_algos.py is fine as there's many objects there anyway many objects and arguably might have been better named uniques from the start)

@deepang17
Copy link
Contributor Author

arr is used at quite a few places, so it will again create conflicts. Please suggest something else. Thank you.

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

I've made some suggestions for some files, for the others I think arr might work (if not, please let me know for which ones it doesn't)

pandas/tests/arithmetic/test_interval.py Outdated Show resolved Hide resolved
pandas/tests/arrays/sparse/test_array.py Outdated Show resolved Hide resolved
pandas/tests/arrays/string_/test_string.py Outdated Show resolved Hide resolved
pandas/tests/arrays/test_datetimelike.py Outdated Show resolved Hide resolved
pandas/tests/dtypes/test_inference.py Show resolved Hide resolved
pandas/tests/extension/base/getitem.py Outdated Show resolved Hide resolved
pandas/tests/indexing/test_check_indexer.py Outdated Show resolved Hide resolved
@deepang17
Copy link
Contributor Author

@MarcoGorelli it's ready to be merged. Thank you for your guidance.

@MarcoGorelli MarcoGorelli self-requested a review March 9, 2021 10:03
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

Hi @deepang17

it's ready to be merged

I admire your confidence :) Still have some comments though, I think some of these names are unclear and that arr should work in those files

pandas/tests/series/test_ufunc.py Outdated Show resolved Hide resolved
pandas/tests/indexing/test_check_indexer.py Outdated Show resolved Hide resolved
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

Looks good to me pending green, cc @jbrockmendel / @jorisvandenbossche for any further comments

@MarcoGorelli
Copy link
Member

Hi @deepang17 - can you fetch and merge upstream/master please?

@MarcoGorelli MarcoGorelli self-requested a review March 10, 2021 09:42
@MarcoGorelli MarcoGorelli self-requested a review March 11, 2021 10:45
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

I went ahead and renamed the rest of these *_array to arr and they seem to be fine, not sure why you said there'd be conflicts - perhaps it was just the fixture?

@deepang17
Copy link
Contributor Author

Great, at that time some other variables were also used with the name arr and one of them produced an error for me hence I thought it will produce an error for all. But now as you have done it, it seems fine.

@MarcoGorelli
Copy link
Member

Thanks @deepang17 !

@MarcoGorelli MarcoGorelli merged commit bf31347 into pandas-dev:master Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Style Code style, linting, code_checks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

STYLE Increase coverage of inconsistent-namespace-usage
4 participants