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 Increase coverage of inconsistent-namespace-usage #39992

Closed
29 tasks done
MarcoGorelli opened this issue Feb 23, 2021 · 23 comments · Fixed by #40286
Closed
29 tasks done

STYLE Increase coverage of inconsistent-namespace-usage #39992

MarcoGorelli opened this issue Feb 23, 2021 · 23 comments · Fixed by #40286
Labels
Code Style Code style, linting, code_checks good first issue
Milestone

Comments

@MarcoGorelli
Copy link
Member

MarcoGorelli commented Feb 23, 2021

If you already have experience contributing, then please leave this issue to newcomers - thanks 👍


In .pre-commit-config.yaml, the check inconsistent-namespace-usage is limited to pandas/tests/frame/. Let's expand it to the rest of pandas/tests/.

The task here is:

  1. make sure you're familiar with the contributing guide
  2. choose a folder from the list below (e.g. series) and add it to the check in .pre-commit-config.yaml. E.g., if you chose series, that would be:
    -   id: inconsistent-namespace-usage
        name: 'Check for inconsistent use of pandas namespace in tests'
        entry: python scripts/check_for_inconsistent_pandas_namespace.py
        language: python
        types: [python]
        files: ^pandas/tests/(frame|series)/
  1. temporarily add tokenize-rt as an additional dependency and --replace as an arg (these will be removed later):
    -   id: inconsistent-namespace-usage
        name: 'Check for inconsistent use of pandas namespace in tests'
        entry: python scripts/check_for_inconsistent_pandas_namespace.py
        language: python
        types: [python]
        files: ^pandas/tests/(frame|series)/
        additional_dependencies: [tokenize-rt]
        args: [--replace]
  1. run this check on all files with pre-commit, i.e.:
  $ pre-commit run inconsistent-namespace-usage --all-files
  [INFO] Initializing environment for local:tokenize-rt.
  [INFO] Installing environment for local.
  [INFO] Once installed this environment will be reused.
  [INFO] This may take a few minutes...
  Check for inconsistent use of pandas namespace in tests.......................Failed
  - hook id: inconsistent-namespace-usage
  - files were modified by this hook
  1. run it again - this time, it should pass
$ pre-commit run inconsistent-namespace-usage --all-files
Check for inconsistent use of pandas namespace in tests.......................Passed

Check that the other hooks still pass - if you enable pre-commit (pre-commit install), then this will happen automatically when you try to commit your changes (see step 6)

  1. revert the changes to .pre-commit-config.yaml (we'll update it once these changes are all so as to avoid a barrage of merge conflicts)
  2. commit your changes, and open a pull request. Check over your changes, make sure they look sensible, let me know if not

Here are the folders in pandas/tests which this check needs expanding to:

  • series
  • scalar
  • resample
  • util
  • tseries
  • dtypes
  • arrays
  • plotting
  • tools
  • indexing
  • strings
  • reductions
  • frame
  • arithmetic
  • config
  • api
  • generic
  • io
  • base
  • window
  • internals
  • reshape
  • computation
  • apply
  • groupby
  • tslibs
  • libs
  • indexes
  • extension

No need to ask for permission to work on this, just leave a comment letting people know which folder(s) from the list above you're working on

@MarcoGorelli MarcoGorelli added this to the 1.3 milestone Feb 23, 2021
@MarcoGorelli MarcoGorelli added the Code Style Code style, linting, code_checks label Feb 23, 2021
@alexprincel
Copy link
Contributor

I ran the script for "extension" and it made no change to files in that folder. I checked my procedure against your example above to make sure this was not the result of a bad manipulation. When changing to series I get the same output as in your post.

@alexprincel
Copy link
Contributor

Currently working on indexes, will submit a PR as there were changes on that folder.

alexprincel added a commit to alexprincel/pandas that referenced this issue Feb 23, 2021
alexprincel added a commit to alexprincel/pandas that referenced this issue Feb 23, 2021
alexprincel added a commit to alexprincel/pandas that referenced this issue Feb 23, 2021
alexprincel added a commit to alexprincel/pandas that referenced this issue Feb 23, 2021
alexprincel added a commit to alexprincel/pandas that referenced this issue Feb 23, 2021
punitvara added a commit to punitvara/pandas that referenced this issue Feb 24, 2021
Fix inconsistent namespace in apply directory.
punitvara added a commit to punitvara/pandas that referenced this issue Feb 24, 2021
Fix inconsistent namespace in apply directory.
punitvara added a commit to punitvara/pandas that referenced this issue Feb 24, 2021
Fix inconsistent namespace in apply directory.
MarcoGorelli pushed a commit that referenced this issue Feb 24, 2021
Fix inconsistent namespace in apply directory.
MarcoGorelli pushed a commit that referenced this issue Feb 24, 2021
Fix inconsistent namespace in apply directory.
alexprincel added a commit to alexprincel/pandas that referenced this issue Feb 25, 2021
@jorisvandenbossche
Copy link
Member

@MarcoGorelli the automatic script only changes it from pd.DataFrame(..) to DataFrame(..) and not the other way around? (as previously, we only agreed to be consistent within a file, not necessarily to change all files to use the non-pd. way)

@MarcoGorelli
Copy link
Member Author

MarcoGorelli commented Feb 25, 2021

@jorisvandenbossche if there's both, it'll choose the non-pd. one. If the file is already consistently using the pd. one, then it'll leave it untouched.

e.g.

import pandas as pd

def test_0():
    pd.DataFrame()

def test_1():
    pd.DataFrame()

would be left untouched (i.e. it would keep being consistent within the file), but

import pandas as pd
from pandas import DataFrame

def test_0():
    pd.DataFrame()

def test_1():
    DataFrame()

would become

import pandas as pd
from pandas import DataFrame

def test_0():
    DataFrame()

def test_1():
    DataFrame()

@alexprincel
Copy link
Contributor

libs and tslibs passed without fixes,

alexprincel added a commit to alexprincel/pandas that referenced this issue Feb 25, 2021
@alexprincel
Copy link
Contributor

computation passed without fixes

@gfyoung gfyoung linked a pull request Mar 4, 2021 that will close this issue
4 tasks
@gfyoung gfyoung removed a link to a pull request Mar 4, 2021
4 tasks
MarcoGorelli pushed a commit that referenced this issue Mar 4, 2021
* Fix styling

* Fix other linting issues

* Revert
MarcoGorelli pushed a commit that referenced this issue Mar 4, 2021
@01-vyom
Copy link
Contributor

01-vyom commented Mar 4, 2021

window, config, generic, and internals passed without fixes

Working on api and arithmetic

@sionedbaker
Copy link
Contributor

As part of pyladies-sprint-March-2021, I looked at dir reductions

@MarcoGorelli
Copy link
Member Author

Nice, thanks @sionedbaker ! It doesn't look like you've opened a pull request though, let us know if you want//need help with that

@Varun270
Copy link
Contributor

Varun270 commented Mar 6, 2021

I am new to open source contribution and I have cloned this repo in my system but I am unable to understand what are the corrections needed to be done. Can anyone help me, I am literally so confused.

@deepang17
Copy link
Contributor

working on tools

MarcoGorelli pushed a commit that referenced this issue Mar 7, 2021
* STYLE: Inconsistent namespace - tools (#39992)

* Pre-Commit Fixes
@MarcoGorelli
Copy link
Member Author

There's only 9 files left now, so if anyone wants to close this issue then please change files to ^pandas/tests/ (though revert the changes to args and additional_dependencies before committing) - then we can close this issue

@deepang17
Copy link
Contributor

Working to close this issue

@deepang17
Copy link
Contributor

image

As you can see in the above image that by replacing pd.unique(mindex) to unique(mindex) we get F821 undefined name 'unique' error.

result = pd.unique(mindex)

Rest all files are resolved.

@deepang17
Copy link
Contributor

One solution is to exclude test_algos.py from checking for inconsistency in pre-commit.
exclude: ^pandas/tests/test_algos.py$

@MarcoGorelli
Copy link
Member Author

There's only 9 files left now, so if anyone wants to close this issue then please change files to ^pandas/tests/ (though revert the changes to args and additional_dependencies before committing) - then we can close this issue

My bad, I missed that @sionedbaker hasn't submitted her PR for reductions yet. @sionedbaker , are you still working on this? If so, let's wait before merging #40286

@sionedbaker
Copy link
Contributor

Hi @MarcoGorelli , very sorry for taking so long to reply, but I had huge problems with pushing my changes to github (due to me having to setup 2FA but I've push changes to my fork now, and then created PR: #40310
I'm not sure if @deepang17 has already solved this, therefore no worries if my PR is surplus to requirements. I very much enjoyed the session on Saturday, but I probably need more practise with submitting this via github as I've found that part the trickiest

@MarcoGorelli
Copy link
Member Author

No worries, thanks for your PR and happy IWD! I recommend the first 3 chapters of the pro-git book, the rest is just practice

@sionedbaker
Copy link
Contributor

Many thanks @MarcoGorelli and thanks for approving the PR. Thanks for the git book, looks good, be really interested in working through some (very simple!) panda issues in the next few weeks, I will keep an eye on the github issues. Be really interested in a further follow up session so will try to keep up to date with pyladies. thanks for all your help

MarcoGorelli added a commit that referenced this issue Mar 11, 2021
* STYLE: Inconsistent namespace - tools (#39992)

* Pre-Commit Fixes

* STYLE: Resolved Inconsistent namespace except test_algos.py - (#39992)

* STYLE: Resolved Inconsistent namespace by excluding test_algos.py

* test_algos.py fixes

* Updated test_algos.py

* STYLE: inconsistent-namespace-usage fixes

* updated test_interval.py

* FIX: Improved varible names

* test_interval.py fixes

* FIX: Resolved confusing variable names

* test_string.py fixes

* getitem.py fixes

* rename more variables to arr

Co-authored-by: Marco Gorelli <marcogorelli@protonmail.com>
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 good first issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants