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: Lint for lists instead of generators in built-in Python functions #18335

Merged
merged 4 commits into from
Nov 19, 2017

Conversation

mroeschke
Copy link
Member

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

Continuation of #18276 plus a lint check for list comprehension in built-in Python functions. The grep will only catch list comprehensions on single lines though. I'm all ears for greps that can catch multi-line list comprehensions or a better grep in general.

@mroeschke
Copy link
Member Author

mroeschke commented Nov 17, 2017

I am not exactly sure why cython won't let me compile either:

_str_reso_map = dict((v, k) for k, v in _reso_str_map.items())

or

_freq_reso_map = dict((v, k) for k, v in _reso_freq_map.items())

in resolution.pyx when _reso_str_map and _reso_freq_map are defined a few lines above them. It will compile if I pass in the list comprehension though.

When I changed the dict function call to {}, cython compiled successfully.

@gfyoung gfyoung added Clean Code Style Code style, linting, code_checks labels Nov 17, 2017
@@ -132,7 +132,7 @@ def setup(self, offset, n_steps):
offset = getattr(offsets, offset)
self.idx = get_index_for_offset(offset(n_steps, **kwargs))
self.df = DataFrame(np.random.randn(len(self.idx), 10), index=self.idx)
self.d = dict([(col, self.df[col]) for col in self.df.columns])
self.d = dict((col, self.df[col]) for col in self.df.columns)
Copy link
Contributor

@max-sixty max-sixty Nov 18, 2017

Choose a reason for hiding this comment

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

could have all these be dict (or set, below) comprehensions now that py2.6 is unsupported

Copy link
Member Author

Choose a reason for hiding this comment

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

So you mean {foo for foo in bar} instead of set(foo for foo in bar)? Sure I don't see why not. Maybe that can be a followup to this.

Copy link
Contributor

Choose a reason for hiding this comment

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

dict(df.items()) should work here

@codecov
Copy link

codecov bot commented Nov 18, 2017

Codecov Report

Merging #18335 into master will decrease coverage by 0.01%.
The diff coverage is 96%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18335      +/-   ##
==========================================
- Coverage   91.38%   91.36%   -0.02%     
==========================================
  Files         164      164              
  Lines       49790    49790              
==========================================
- Hits        45501    45492       -9     
- Misses       4289     4298       +9
Flag Coverage Δ
#multiple 89.17% <88%> (ø) ⬆️
#single 39.49% <42%> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/io/clipboards.py 100% <ø> (ø) ⬆️
pandas/plotting/_core.py 82.45% <0%> (ø) ⬆️
pandas/tseries/holiday.py 93.1% <100%> (ø) ⬆️
pandas/plotting/_converter.py 65.25% <100%> (ø) ⬆️
pandas/core/indexing.py 92.8% <100%> (ø) ⬆️
pandas/core/generic.py 95.73% <100%> (ø) ⬆️
pandas/core/sparse/frame.py 94.78% <100%> (ø) ⬆️
pandas/core/indexes/interval.py 92.85% <100%> (ø) ⬆️
pandas/io/sql.py 94.8% <100%> (ø) ⬆️
pandas/core/internals.py 94.54% <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 cfad581...c91410f. Read the comment docs.

@codecov
Copy link

codecov bot commented Nov 18, 2017

Codecov Report

Merging #18335 into master will decrease coverage by 0.01%.
The diff coverage is 96%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18335      +/-   ##
==========================================
- Coverage   91.36%   91.34%   -0.02%     
==========================================
  Files         164      164              
  Lines       49790    49790              
==========================================
- Hits        45489    45480       -9     
- Misses       4301     4310       +9
Flag Coverage Δ
#multiple 89.14% <88%> (ø) ⬆️
#single 39.53% <42%> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/io/clipboards.py 100% <ø> (ø) ⬆️
pandas/plotting/_core.py 82.45% <0%> (ø) ⬆️
pandas/tseries/holiday.py 93.1% <100%> (ø) ⬆️
pandas/io/parsers.py 95.59% <100%> (ø) ⬆️
pandas/stats/moments.py 71.19% <100%> (ø) ⬆️
pandas/io/sql.py 94.8% <100%> (ø) ⬆️
pandas/core/sparse/frame.py 94.78% <100%> (ø) ⬆️
pandas/core/reshape/concat.py 97.58% <100%> (ø) ⬆️
pandas/core/panelnd.py 98.21% <100%> (ø) ⬆️
pandas/core/common.py 93% <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 1798c9d...3572626. Read the comment docs.

@jreback
Copy link
Contributor

jreback commented Nov 19, 2017

can you rebase

@@ -132,7 +132,7 @@ def setup(self, offset, n_steps):
offset = getattr(offsets, offset)
self.idx = get_index_for_offset(offset(n_steps, **kwargs))
self.df = DataFrame(np.random.randn(len(self.idx), 10), index=self.idx)
self.d = dict([(col, self.df[col]) for col in self.df.columns])
self.d = dict((col, self.df[col]) for col in self.df.columns)
Copy link
Contributor

Choose a reason for hiding this comment

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

dict(df.items()) should work here

@@ -202,7 +202,7 @@ class read_json_lines(object):
def setup(self):
self.N = 100000
self.C = 5
self.df = DataFrame(dict([('float{0}'.format(i), randn(self.N)) for i in range(self.C)]))
self.df = DataFrame(dict(('float{0}'.format(i), randn(self.N)) for i in range(self.C)))
self.df.to_json(self.fname,orient="records",lines=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

could use the comprehension

{('float{0}'.format(i), randn(self.N)) for i in range(self.C))}

@@ -101,7 +101,7 @@ def conv(i):


def _sanitize_and_check(indexes):
kinds = list(set([type(index) for index in indexes]))
kinds = list(set(type(index) for index in indexes))
Copy link
Contributor

Choose a reason for hiding this comment

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

this could be a set comprehension
{type(index) for index in indexes}

prob more of these as well

@mroeschke
Copy link
Member Author

Rebased. I can create a follow up issue/PR to replace instances of set(foo for foo in iterator) with {foo for foo in iterator} where appropriate (likewise for lists and dicts)

@jreback jreback added this to the 0.22.0 milestone Nov 19, 2017
@jreback jreback merged commit 410ad37 into pandas-dev:master Nov 19, 2017
@jreback
Copy link
Contributor

jreback commented Nov 19, 2017

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean Code Style Code style, linting, code_checks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants