-
-
Notifications
You must be signed in to change notification settings - Fork 18.3k
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
STYLE: #22885, Moved all setup imports to bottom of benchmark files and added noqa: F401 to each #22947
Conversation
Hello @jerodestapa! Thanks for submitting the PR.
|
There was a problem hiding this 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.
Thanks for the work on this @jerodestapa |
For the pytest issue, 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. |
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 |
can you rebase |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
My apologies on the wait, @jreback. Let me know if anything else needs fixing. Thanks. |
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):
This will delete the history of your changes, but should leave your changes and nothing else. |
…s and added noqa: F401 to each
7572888
to
a04762d
Compare
Thanks, @datapythonista. I must have done something weird when rebasing. I just reset and pushed again. |
this looks fine. can you run the asv's just to be sure things still work. |
@jreback: The asv's are still running, but it looks like the four files that updated when I rebased ( |
I think the PR is ok as it is. Not sure why I think this is ready to be merged, thanks for the work @jerodestapa |
Awesome. Thanks for all the help, guys. This was my first PR for pandas, and it's just exciting to contribute. Thanks, again. |
Hopefully not the last one @jerodestapa ;) |
git diff upstream/master -u -- "*.py" | flake8 --diff
#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 inasv_bench
:These may be expected, but I wanted to mention them here just in case they need addressing.
Also, running
pytest
, I got this error: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!