STYLE use ast.parse instead of regex to fixup inconsistencies in namespace #39690

MarcoGorelli · 2021-02-09T08:52:15Z

tests added / passed
Ensure all linting tests pass, see here for how to run them

The regular expression wasn't working too well, this solution should be better. Also, like this, you can pass --replace and have the inconsistencies fixed up for you (though I haven't passed this by default as it may be confusing)

Just running on pandas/tests/frame/ here, can expand to the rest in separate PRs if this is alright

dsaxton

Thanks @MarcoGorelli. The logic of how this works is a bit over my head to be honest, but looks like it's catching a lot that was missed previously 👍

dsaxton · 2021-02-09T21:50:06Z

scripts/check_for_inconsistent_pandas_namespace.py

@@ -7,52 +7,97 @@
 This is meant to be run as a pre-commit hook - to run it manually, you can do:

    pre-commit run inconsistent-namespace-usage --all-files
+
+To automatically fixup a given file, you can pass `--replace`, e.g.


Is there a preference for how to fix the file if both usages occur?

currently I'm just removing the pd. prefix if both occur

jbrockmendel · 2021-02-09T23:19:07Z

@momisbestfriend can you take a look? IIRC you did a lot of the ast-based checks

MarcoGorelli · 2021-02-10T11:12:27Z

@momisbestfriend can you take a look? IIRC you did a lot of the ast-based checks

@ShaharNaveh

jorisvandenbossche · 2021-02-10T14:27:14Z

pandas/tests/frame/test_constructors.py

@@ -753,7 +754,7 @@ def test_constructor_extension_scalar_data(self, data, dtype):
        assert df["a"].dtype == dtype
        assert df["b"].dtype == dtype

-        arr = pd.array([data] * 2, dtype=dtype)
+        arr = pd_array([data] * 2, dtype=dtype)


What's the advantage of using such an alias instead of simply using pd.array ?

that in this file there is already import array, so having pd.array as well would fail the check

Yes, but I meant from a developer (code writer / reader) perspective ;)
I understand that the check has difficulties with it, but then I would say to either fix the check or ignore "array" in the check if it's not easy to fix (IMO array in tests should always be used as pd.array, so it might worth a special case in the checking code)

sure, I can add it to EXCLUDE (along with eval and np)

jreback

lgtm ex @jorisvandenbossche comments

MarcoGorelli · 2021-02-10T16:36:27Z

lgtm ex @jorisvandenbossche comments

Thanks - I believe these were addressed in d7825aa, but Joris do let me know if you had something different in mind

jbrockmendel · 2021-02-10T16:40:09Z

i think i got into the habit of importing array as pd_array bc there are a number of places where we use "array" as a variable name (or in principle could be using the stdlib array module) and name overlap of that form bothers me more than most

ShaharNaveh · 2021-02-10T19:07:12Z

@momisbestfriend can you take a look? IIRC you did a lot of the ast-based checks

Yes correct I did a lot of AST checks:)

This script looks amazing, LGTM:)

…space-ast

MarcoGorelli · 2021-02-14T15:42:14Z

This script looks amazing

Thanks - I learned loads by studying the pyupgrade source code

jreback · 2021-02-15T22:31:02Z

@MarcoGorelli can you merge master and ping on green. i think this is fine (and followup PRs to add coverage are good).

…space-ast

MarcoGorelli · 2021-02-16T15:12:52Z

Sure, have merged - @jreback

EDIT I'll try re-writing this slightly so it's just standard library, then the same local virtualenv as is used for lots of other checks can be reused

UPDATE

Now, tokenize-rt is only required if using the --replace flag. This way, the pre-commit hook can re-use the same virtualenv as other local Python hooks

…space-ast

pep8speaks · 2021-02-16T15:51:39Z

Hello @MarcoGorelli! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-02-18 12:12:43 UTC

…pandas into inconsistent-namespace-ast

…space-ast

jreback · 2021-02-19T01:12:01Z

thanks @MarcoGorelli pls open an issue to fix the non-checked parts of the codebase.

MarcoGorelli · 2021-02-19T07:16:47Z

thanks @MarcoGorelli pls open an issue to fix the non-checked parts of the codebase.

Thanks - I'm running a little event with PyLadies London on the 6th of March, so I'll get participants to work on this then

MarcoGorelli requested review from dsaxton and jbrockmendel February 9, 2021 08:52

lithomas1 added the Code Style Code style, linting, code_checks label Feb 9, 2021

dsaxton reviewed Feb 9, 2021

View reviewed changes

jorisvandenbossche requested changes Feb 10, 2021

View reviewed changes

jreback added this to the 1.3 milestone Feb 10, 2021

jreback approved these changes Feb 10, 2021

View reviewed changes

MarcoGorelli added 4 commits February 12, 2021 16:53

use ast.parse to fixup inconsistencies in namespace

d370b4a

use Offset namedtuple

65dbc19

strict typing

285abca

exclude array

1f6eba0

MarcoGorelli force-pushed the inconsistent-namespace-ast branch from d7825aa to 1f6eba0 Compare February 12, 2021 16:55

Merge remote-tracking branch 'upstream/master' into inconsistent-name…

22ae54b

…space-ast

MarcoGorelli mentioned this pull request Feb 14, 2021

Extend check for inconsistent pandas namespace usage #38093

Closed

Merge remote-tracking branch 'upstream/master' into inconsistent-name…

6b1bd7a

…space-ast

MarcoGorelli requested a review from jreback February 16, 2021 15:19

MarcoGorelli added 2 commits February 16, 2021 15:48

replace next token

02e89bd

Merge remote-tracking branch 'upstream/master' into inconsistent-name…

10a49f1

…space-ast

fixup

a673ec2

MarcoGorelli marked this pull request as draft February 16, 2021 17:02

MarcoGorelli marked this pull request as ready for review February 16, 2021 18:33

MarcoGorelli marked this pull request as draft February 16, 2021 18:35

MarcoGorelli added 2 commits February 16, 2021 20:44

dont require tokenize-rt for pre-commit hook

a8854a2

Merge branch 'inconsistent-namespace-ast' of github.com:MarcoGorelli/…

b6b3de0

…pandas into inconsistent-namespace-ast

MarcoGorelli marked this pull request as ready for review February 16, 2021 20:55

Merge remote-tracking branch 'upstream/master' into inconsistent-name…

3fdf446

…space-ast

jreback merged commit de55c3d into pandas-dev:master Feb 19, 2021

MarcoGorelli deleted the inconsistent-namespace-ast branch February 19, 2021 07:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

STYLE use ast.parse instead of regex to fixup inconsistencies in namespace #39690

STYLE use ast.parse instead of regex to fixup inconsistencies in namespace #39690

MarcoGorelli commented Feb 9, 2021 •

edited

Loading

dsaxton left a comment

dsaxton Feb 9, 2021

MarcoGorelli Feb 10, 2021

jbrockmendel commented Feb 9, 2021

MarcoGorelli commented Feb 10, 2021

jorisvandenbossche Feb 10, 2021

MarcoGorelli Feb 10, 2021 •

edited

Loading

jorisvandenbossche Feb 10, 2021

MarcoGorelli Feb 10, 2021

jorisvandenbossche Feb 10, 2021

jreback left a comment

MarcoGorelli commented Feb 10, 2021

jbrockmendel commented Feb 10, 2021

ShaharNaveh commented Feb 10, 2021

MarcoGorelli commented Feb 14, 2021

jreback commented Feb 15, 2021

MarcoGorelli commented Feb 16, 2021 •

edited

Loading

pep8speaks commented Feb 16, 2021 •

edited

Loading

jreback commented Feb 19, 2021

MarcoGorelli commented Feb 19, 2021

STYLE use ast.parse instead of regex to fixup inconsistencies in namespace #39690

STYLE use ast.parse instead of regex to fixup inconsistencies in namespace #39690

Conversation

MarcoGorelli commented Feb 9, 2021 • edited Loading

dsaxton left a comment

Choose a reason for hiding this comment

dsaxton Feb 9, 2021

Choose a reason for hiding this comment

MarcoGorelli Feb 10, 2021

Choose a reason for hiding this comment

jbrockmendel commented Feb 9, 2021

MarcoGorelli commented Feb 10, 2021

jorisvandenbossche Feb 10, 2021

Choose a reason for hiding this comment

MarcoGorelli Feb 10, 2021 • edited Loading

Choose a reason for hiding this comment

jorisvandenbossche Feb 10, 2021

Choose a reason for hiding this comment

MarcoGorelli Feb 10, 2021

Choose a reason for hiding this comment

jorisvandenbossche Feb 10, 2021

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

MarcoGorelli commented Feb 10, 2021

jbrockmendel commented Feb 10, 2021

ShaharNaveh commented Feb 10, 2021

MarcoGorelli commented Feb 14, 2021

jreback commented Feb 15, 2021

MarcoGorelli commented Feb 16, 2021 • edited Loading

UPDATE

pep8speaks commented Feb 16, 2021 • edited Loading

Comment last updated at 2021-02-18 12:12:43 UTC

jreback commented Feb 19, 2021

MarcoGorelli commented Feb 19, 2021

MarcoGorelli commented Feb 9, 2021 •

edited

Loading

MarcoGorelli Feb 10, 2021 •

edited

Loading

MarcoGorelli commented Feb 16, 2021 •

edited

Loading

pep8speaks commented Feb 16, 2021 •

edited

Loading