-
Notifications
You must be signed in to change notification settings - Fork 8
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
Use from_parent #8
Use from_parent #8
Conversation
This is the first time I touch a pytest plugin, so please review it before merging 🙏 |
It seems this plugin is very similar to pytest flakes. Do you know which one came first? That plugin has the same issue asmeurer/pytest-flakes#26. I tried applying your fix here to pytest-flakes, replacing
But I remove
|
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.
Thanks! I added some comments for possible simplifications.
pytest-flakes is from 2013, pytest-mccabe from 2015. And indeed it's based on pytest-flakes' code!
Could you show your full patch somewhere (e.g. in a draft PR for pytest-flakes)? |
@fschulze is transferring ownership of pytest-flakes to me. Once that happens, I will push up what I have (but it's basically this PR except with the |
This is based on The-Compiler/pytest-mccabe#8. However, the fix doesn't work, and I'm not sure how to fix it. I either get the error TypeError: from_parent() got an unexpected keyword argument 'flakesignore' or if I remove flakesignore from the super.from_parent call, I get TypeError: __init__() missing 1 required positional argument: 'flakesignore'
See the most recent commit at asmeurer/pytest-flakes#30 |
I see travis failing, but when I execute pytest locally it runs just fine. Do you have any idea why? To the issue itself: As I don't understand the "from_parent" (which is the core part of this PR), I don't understand the issue. |
Thanks for the update! I think it's failing because only old pytest versions get tested, which are now not supported anymore. I'll need to update the CI configuration (and probably also switch to GitHub Actions while I'm at it) so that this actually gets tested against a newer pytest version. It'll likely take me a couple of days (or perhaps weeks) though, as I'm busy with some other stuff at the moment. If you have time to help with that as well, let me know and I can give you some pointers, though! |
Ah, I see. I'm not super excited about fixing CI pipelines ... let's see, maybe I'll give it a try 😄 Should I make another PR for that or would it fit best in here? |
I realized I could almost copy-paste a config I wrote for pytest-xvfb, so that's what I ended up doing in b5f2cd6. I also cleaned up various code and updated supported versions in 42c7fc1. Thanks for the fix and for getting the ball rolling again! So from my point of view, things would be ready for a 2.0 release - any objections? |
Oh, nice! Thanks for merging :-) |
You're welcome! So, anything missing from your point of view or anything else you'd want to contribute before a release? Otherwise I'd push a v2.0 tag and hope the automation takes care of the rest :) |
It looks fine to me - but as I said, take this with caution. That is the first pytest plugin I'm contributing to :-) |
Alright! I published pytest-mccabe 2.0 - if anything is amiss, a v2.1 is just a version bump, commit, tag and "git push" away as the automation worked out just fine 🎉 |
Closes #7