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

Fix remove_class when methods have decorators #76

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Code0x58
Copy link
Contributor

@Code0x58 Code0x58 commented Dec 7, 2024

Reopens #50 to target main instead of the now removed develop branch

pass


@property
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Member

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

Copy link
Member

@nicoddemus nicoddemus Dec 7, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants