Skip to content

Conversation

rockg
Copy link
Contributor

@rockg rockg commented Jan 16, 2016

yapf with manual modifications for error strings and bad yapf formatting.

@jreback jreback added the Code Style Code style, linting, code_checks label Jan 16, 2016
@jreback jreback added this to the 0.18.0 milestone Jan 16, 2016
@rockg
Copy link
Contributor Author

rockg commented Jan 16, 2016

No github diffs again!

There are some pieces of code that I think can be removed but want to confirm:
index.py i/j are not used anywhere.
groupby.py how can this work? agg_func is not defined.

@jreback
Copy link
Contributor

jreback commented Jan 16, 2016

index i/j ok to remove

@jreback
Copy link
Contributor

jreback commented Jan 16, 2016

yeh that looks not tested, it for a transform on a Panel or greater. I think just raise NotImplemented (if the ndim >= 3) & remove the 2nd clause (where you noted). If nothing breaks in the test suite then ok!

@rockg
Copy link
Contributor Author

rockg commented Jan 16, 2016

I assume it should be transform_func(result[:, :, i], chunk, comp_ids, accum)

@rockg
Copy link
Contributor Author

rockg commented Jan 16, 2016

k, I can do that.

@jorisvandenbossche
Copy link
Member

@rockg I would keep style changes and code clean-up (as in removal of unused code or a change of actual code) strictly separated (at least in a separate commit). Just to be sure, in case we overlooked something, then it is not burried in a gigantic commit for which you can't see the diff.

@rockg
Copy link
Contributor Author

rockg commented Jan 16, 2016

The main goal of this exercise is to make flake8 pass and cleanup some style were necessary. What I cleaned out were unused variables reported by flake8 and, as a result, I don't see a problem including that here.

@wesm
Copy link
Member

wesm commented Jan 16, 2016

@rockg I've been putting # noqa on these lines and adding a TODO comment to come back and investigate

@wesm
Copy link
Member

wesm commented Jan 16, 2016

This is especially important in unit tests where I suspect that there are some test cases that may have skipped some intended checks

@rockg
Copy link
Contributor Author

rockg commented Jan 16, 2016

Okay, that seems reasonable.

@jreback
Copy link
Contributor

jreback commented Jan 19, 2016

ok, rebasing core/groupby.py is going to be painful for #11841 but go ahead (my touches are mostly reorg anyhow)

@wesm
Copy link
Member

wesm commented Jan 19, 2016

Is this good to merge?

@wesm
Copy link
Member

wesm commented Jan 19, 2016

I'll leave this to @jreback with the pending rebase

@jreback
Copy link
Contributor

jreback commented Jan 19, 2016

if u want to remove groupby.py from this would be great (I'll catch that in resample fixes)

otherwise lgtm

@wesm
Copy link
Member

wesm commented Jan 20, 2016

thanks! we're almost there =)

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