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

STYLE: #22885, Moved all setup imports to bottom of benchmark files and added noqa: F401 to each #22947

Merged
merged 1 commit into from
Oct 10, 2018

Conversation

jerod-estapa
Copy link
Contributor

#22885

Following the suggestions of @datapythonista, I moved all the setup imports for the benchmark files to the end of each file and added a # noqa: F401 like so:

from .pandas_vb_common import setup # noqa: F401

Now, flake8 only returns the following two errors in asv_bench:

./benchmarks/strings.py:95:9: E731 do not assign a lambda expression, use a def
./benchmarks/io/excel.py:6:1: F401 '..pandas_vb_common.BaseIO' imported but unused

These may be expected, but I wanted to mention them here just in case they need addressing.

Also, running pytest, I got this error:

usage: pytest [options] [file_or_dir] [file_or_dir] [...]
pytest: error: unrecognized arguments: --strict-data-files
  inifile: /pandas/setup.cfg
  rootdir: /pandas

I found a discussion on it at #22700, but there didn't seem to be a definitive solution. Open to guidance on this.

Please let me know if any corrections are needed. Thanks!

@pep8speaks
Copy link

Hello @jerodestapa! Thanks for submitting the PR.

@datapythonista datapythonista added Code Style Code style, linting, code_checks Benchmark Performance (ASV) benchmarks labels Oct 2, 2018
Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

lgtm. The approach is not ideal of course, but I think it's the best we can do.

Once this and #22863 are merged, we can activate the linting of the benchmarks.

@datapythonista
Copy link
Member

Thanks for the work on this @jerodestapa

@TomAugspurger
Copy link
Contributor

For the pytest issue, pytest pandas should work.

I think we're doing something wrong with how we register new command line options, but I'm not familiar enough with pytest plugins to know how to fix it.

@jerod-estapa
Copy link
Contributor Author

You bet, @datapythonista. Thanks for suggestions. I agree it's less than ideal, but as you say, it's the best solution given the constraints around flake8.

@TomAugspurger, it's not liking something about setup.cfg and the default option addopts = --strict-data-files --durations=10, but it may just be a local issue on my end. I'll let you know if I figure it out, though.

@jreback
Copy link
Contributor

jreback commented Oct 7, 2018

can you rebase

@codecov
Copy link

codecov bot commented Oct 9, 2018

Codecov Report

Merging #22947 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #22947      +/-   ##
==========================================
- Coverage   92.19%   92.19%   -0.01%     
==========================================
  Files         169      169              
  Lines       50904    50911       +7     
==========================================
+ Hits        46933    46939       +6     
- Misses       3971     3972       +1
Flag Coverage Δ
#multiple 90.61% <ø> (-0.01%) ⬇️
#single 42.3% <ø> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/core/internals/concat.py 98% <0%> (-0.38%) ⬇️
pandas/core/dtypes/dtypes.py 95.58% <0%> (+0.03%) ⬆️

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 ca7d518...a04762d. Read the comment docs.

@jerod-estapa
Copy link
Contributor Author

My apologies on the wait, @jreback. Let me know if anything else needs fixing.

Thanks.

@datapythonista
Copy link
Member

Not sure if you've seen it @jerodestapa, but seems like you've got unrelated changes in this PR.

If you don't find a better way to fix it, usually the next steps could fix it (in the PR branch):

  • git fetch upstream
  • git merge upstream/master
  • git reset --soft upstream/master
  • git commit -m "your pr comment"
  • git push -f

This will delete the history of your changes, but should leave your changes and nothing else.

@jerod-estapa
Copy link
Contributor Author

Thanks, @datapythonista. I must have done something weird when rebasing. I just reset and pushed again.

@jreback jreback added this to the 0.24.0 milestone Oct 10, 2018
@jreback
Copy link
Contributor

jreback commented Oct 10, 2018

this looks fine. can you run the asv's just to be sure things still work.

@jerod-estapa
Copy link
Contributor Author

@jreback: The asv's are still running, but it looks like the four files that updated when I rebased (indexing.py, join_merge.py, panel_ctor.py, and panel_methods.py) still have the from .pandas_vb_common import Panel line instead of importing Panel from pandas. Should I remove those lines and make another commit, or rebase again?

@datapythonista
Copy link
Member

I think the PR is ok as it is. Not sure why Panel was being imported from pandas_vb_common.py instead of directly, but as your changes already fix the linting, and Panel will be removed soon anyway, I think it's worth just leaving it as it was.

I think this is ready to be merged, thanks for the work @jerodestapa

@datapythonista datapythonista merged commit 296c251 into pandas-dev:master Oct 10, 2018
@jerod-estapa
Copy link
Contributor Author

Awesome. Thanks for all the help, guys. This was my first PR for pandas, and it's just exciting to contribute. Thanks, again.

@datapythonista
Copy link
Member

Hopefully not the last one @jerodestapa ;)

@jerod-estapa jerod-estapa deleted the setup-linting-fix branch October 10, 2018 17:24
@h-vetinari h-vetinari mentioned this pull request Oct 11, 2018
tm9k1 pushed a commit to tm9k1/pandas that referenced this pull request Nov 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Benchmark Performance (ASV) benchmarks Code Style Code style, linting, code_checks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

STYLE: Fix lint error "F811 redefinition of unused 'setup'" in benchmarks
5 participants