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

CLN: Replace comprehensions list/set/dict functions with corresponding symbols #18383

Merged
merged 4 commits into from
Nov 24, 2017

Conversation

mroeschke
Copy link
Member

@mroeschke mroeschke commented Nov 20, 2017

  • tests passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff

xref @jreback's comment and @maxim-lian comment, replace the following where appropriate:

list(x for x in iterator) --> [x for x in iterator]
set(x for x in iterator) --> {x for x in iterator}
dict((x,x) for x in iterator) --> {x: x for x in iterator}

@jreback jreback added the Code Style Code style, linting, code_checks label Nov 20, 2017
@codecov
Copy link

codecov bot commented Nov 20, 2017

Codecov Report

Merging #18383 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18383      +/-   ##
==========================================
- Coverage   91.36%   91.36%   -0.01%     
==========================================
  Files         164      164              
  Lines       49718    49814      +96     
==========================================
+ Hits        45426    45513      +87     
- Misses       4292     4301       +9
Flag Coverage Δ
#multiple 89.16% <97.95%> (+0.02%) ⬆️
#single 39.55% <36.73%> (-0.12%) ⬇️
Impacted Files Coverage Δ
pandas/util/testing.py 100% <ø> (ø) ⬆️
pandas/io/clipboards.py 100% <ø> (ø) ⬆️
pandas/io/json/json.py 100% <ø> (ø) ⬆️
pandas/tseries/offsets.py 96.92% <100%> (+0.22%) ⬆️
pandas/core/reshape/concat.py 97.58% <100%> (ø) ⬆️
pandas/tseries/holiday.py 93.1% <100%> (ø) ⬆️
pandas/io/parsers.py 95.59% <100%> (ø) ⬆️
pandas/core/ops.py 91.77% <100%> (ø) ⬆️
pandas/core/indexes/interval.py 92.52% <100%> (ø) ⬆️
pandas/core/reshape/melt.py 96.19% <100%> (ø) ⬆️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1647a72...ce932a6. Read the comment docs.

@codecov
Copy link

codecov bot commented Nov 20, 2017

Codecov Report

Merging #18383 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18383      +/-   ##
==========================================
- Coverage   91.35%   91.33%   -0.03%     
==========================================
  Files         163      163              
  Lines       49695    49695              
==========================================
- Hits        45401    45391      -10     
- Misses       4294     4304      +10
Flag Coverage Δ
#multiple 89.13% <97.95%> (-0.01%) ⬇️
#single 39.66% <36.73%> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/io/clipboards.py 100% <ø> (ø) ⬆️
pandas/io/json/json.py 100% <ø> (ø) ⬆️
pandas/util/testing.py 100% <ø> (ø) ⬆️
pandas/core/indexes/base.py 96.43% <100%> (ø) ⬆️
pandas/plotting/_converter.py 65.25% <100%> (ø) ⬆️
pandas/core/panelnd.py 98.21% <100%> (ø) ⬆️
pandas/core/common.py 93% <100%> (ø) ⬆️
pandas/tseries/offsets.py 97% <100%> (ø) ⬆️
pandas/core/internals.py 94.47% <100%> (ø) ⬆️
pandas/core/reshape/concat.py 97.58% <100%> (ø) ⬆️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6eac0b...371c22f. Read the comment docs.

@mroeschke
Copy link
Member Author

ping all green

ci/lint.sh Outdated

# Example: Avoid `list(i for i in some_iterator)` in favor of `[i for i in some_iterator]`
#
# Check the following functions:
Copy link
Contributor

Choose a reason for hiding this comment

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

don''t you need to specify the directory?

e.g. on current master

(pandas) bash-3.2$ grep -R --include="*.py*" -E "(list|dict|[^frozen]set)\([^\{)=]* for .* in [^}]*\)" pandas
pandas/_libs/parsers.pyx:        dtypes = set(a.dtype for a in arrs)
pandas/_version.py:        tags = set(r for r in refs if re.search(r'\d', r))
pandas/core/dtypes/concat.py:        fill_values = set(c.fill_value for c in to_concat)
pandas/core/groupby.py:                            names = set(v.name for v in values)
pandas/core/indexes/base.py:        names = set(obj.name for obj in to_concat)
pandas/core/indexes/interval.py:        if not len(set(i.closed for i in to_concat if len(i))) == 1:
pandas/core/internals.py:        ndim = set(b.ndim for b in blocks)
pandas/core/internals.py:            if len(set(b.dtype for b in blocks)) != 1:
pandas/core/reshape/concat.py:            if not len(set(idx.nlevels for idx in indexes)) == 1:
pandas/io/parsers.py:        return set(i for i, name in enumerate(names)
pandas/io/pytables.py:        axis = list(set(t.non_index_axes[0][0] for t in tbls))[0]
pandas/tests/groupby/test_whitelist.py:    results = set(v for v in dir(grp) if not v.startswith('_'))

Copy link
Member Author

Choose a reason for hiding this comment

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

I was wanting to lint the entire project (including python scripts in the other directories, e.g docs) and Travis was linting everything.

I can change it to just lint the pandas directory if you want.

Copy link
Contributor

Choose a reason for hiding this comment

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

no u need to specify a dir . is ok

the command doesn’t work atm

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes it looks my grep is too restrictive here.

Admittedly my grep-foo is a bit too weak for this lint. I am not sure how to write a grep to catching matching parenthesis, list|set|dict( to the matching ), and there are edge cases that are hard to catch, e.g. list(set comprehension) is okay, dict() can accept = that is assigned to a list comprehension, etc.

I can create a new issue for this unless someone with more grep experience can chime in.

@mroeschke
Copy link
Member Author

I created issue #18464 to follow up on the lint statement for this pattern.

@@ -606,11 +606,11 @@ def git_versions_from_keywords(keywords, tag_prefix, verbose):
if verbose:
print("keywords are unexpanded, not using")
raise NotThisMethod("unexpanded keywords, not a git-archive tarball")
refs = set(r.strip() for r in refnames.strip("()").split(","))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure it is good idea to clean up versioneer since this is just a file from another project. Maybe this should be excluded?

Copy link
Contributor

Choose a reason for hiding this comment

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

it probably doesn't matter much. we already replace some formatting code here as well.

@jreback jreback added this to the 0.22.0 milestone Nov 24, 2017
@jreback jreback merged commit 4fce784 into pandas-dev:master Nov 24, 2017
@jreback
Copy link
Contributor

jreback commented Nov 24, 2017

thanks @mroeschke

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.

None yet

3 participants