-
Notifications
You must be signed in to change notification settings - Fork 27
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
Fix remove_class when methods have decorators #76
base: main
Are you sure you want to change the base?
Conversation
pass | ||
|
||
|
||
@property |
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.
This seems weird to me, wouldn't be better to keep the entire class if we find a decorator?
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.
I haven't looks at this in a while (I suppose the nature of a one-shot tool), so my recollection might be a bit off, but:
It does seem weird, particularly with the choice of the property decorator as an example, as what simple transformation would you do for property
, let alone generic things?
Some decorators work transparently with/without classes (e.g. functools.cache
), so sometimes leaving the decorator is fine/good. Then there's all the other cases, which are probably best left to be sorted by hand - at least leaving the decorator shows you what was there so you can work from there, instead of using a git diff
etc. So keeping the decorators seems better to me than dropping it, as I think currently happens (have yet to confirm, but I imagine that was what inspired me to make this change when I used the tool ages ago).
Flat out avoiding transforming a class when a potentially non-trivial/breaking change is detected does sound like a decent option, although I'm not sure it is what happens in the tool at present. In the use case I recall, I didn't mind having to put in a fix for the odd small non-trivial/breaking bit after the tool handled a large amount of trivial cases, so I would like the option to keep the "I'll do most of the work, you just fix the odd like leftover" tool behaviour.
Going through that makes me think think that this is at least an improvement, with potential for further improvement down the line as well
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.
property is the prime example of removing the class Is plain wrong
Any descriptor is simply plain broken without a class
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.
Yeah I agree, if we have a decorator, we should not attempt to remove the class itself... we can leave the class there, and still convert self.assert*
methods to assert
, assertRaises
to pytest.raises
, etc, but leave the class intact.
Reopens #50 to target
main
instead of the now removeddevelop
branch