Fix remove_class when methods have decorators#76
Fix remove_class when methods have decorators#76Code0x58 wants to merge 1 commit intopytest-dev:mainfrom
Conversation
| pass | ||
|
|
||
|
|
||
| @property |
There was a problem hiding this comment.
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.
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.
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.
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
maininstead of the now removeddevelopbranch