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

MAINT: Expand lint for *.py #14516

Merged

Conversation

gfyoung
Copy link
Member

@gfyoung gfyoung commented Oct 27, 2016

Expand lint to include all Python files, excluding the directories pandas/rpy (deprecated and soon to be removed) and pandas/src (C code).

@jreback
Copy link
Contributor

jreback commented Oct 27, 2016

are flake8 options not strict enough? IOW why were these not caught before?

@jreback jreback added the Code Style Code style, linting, code_checks label Oct 27, 2016
@jreback jreback added this to the 0.19.1 milestone Oct 27, 2016
@codecov-io
Copy link

codecov-io commented Oct 27, 2016

Current coverage is 85.26% (diff: 91.66%)

Merging #14516 into master will not change coverage

@@             master     #14516   diff @@
==========================================
  Files           140        140          
  Lines         50672      50672          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits          43207      43207          
  Misses         7465       7465          
  Partials          0          0          

Powered by Codecov. Last update d7fb5bd...e7429aa

@gfyoung
Copy link
Member Author

gfyoung commented Oct 27, 2016

@jreback : TBH, I have no idea. I didn't really change anything with the command flags. But if it works, that's work counts I guess. 😄

@gfyoung
Copy link
Member Author

gfyoung commented Oct 27, 2016

@jreback : Is this good to merge since Travis is passing?

@@ -2194,16 +2194,17 @@ def _handle_usecols(self, columns, usecols_key):
usecols_key is used if there are string usecols.
"""
if self.usecols is not None:
if any([isinstance(u, string_types) for u in self.usecols]):
if any([isinstance(usecol, string_types)
for usecol in self.usecols]):
Copy link
Member

Choose a reason for hiding this comment

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

is this needed to pass the tests? (I can't imagine we nowhere use i in a iteration) as I actually find the one-liner better

Copy link
Member Author

Choose a reason for hiding this comment

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

@jorisvandenbossche : The u shadows the u import from pandas.compat. I don't see any problem with making this expanded as usecol.

Copy link
Member

Choose a reason for hiding this comment

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

ah ok, could also use c or col

It's not a 'problem', but IMO a multiline list comprehension or multiline if condition is always less clear than a single line one (if you are not too brief to get it on one line).

Copy link
Member Author

Choose a reason for hiding this comment

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

That works.

@jorisvandenbossche jorisvandenbossche modified the milestones: 0.20.0, 0.19.1 Oct 27, 2016
@gfyoung
Copy link
Member Author

gfyoung commented Oct 27, 2016

@jorisvandenbossche : Any reason why you moved it to 0.20.0? This isn't an API change.

@jorisvandenbossche
Copy link
Member

jorisvandenbossche commented Oct 27, 2016

It doesn't matter to merge now or not, that just indicates that it is not essential to backport

@gfyoung
Copy link
Member Author

gfyoung commented Oct 27, 2016

Ah, okay. Good to know!

@jorisvandenbossche jorisvandenbossche merged commit 096d886 into pandas-dev:master Oct 28, 2016
@jorisvandenbossche
Copy link
Member

@gfyoung thanks!

@gfyoung gfyoung deleted the py-flake8-expand branch October 28, 2016 17:20
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.

4 participants