Skip to content

Fix remove_class when methods have decorators #76

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.

Sign up for GitHub

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

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

Fix remove_class when methods have decorators #76

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions tests/fixtures/remove_class/dedent_in.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
class TestAssertNotEqual(TestCase):

def test_you(self):
pass


@property
def test_me(self):
[
item
for item in sequence
if item
]
12 changes: 12 additions & 0 deletions tests/fixtures/remove_class/dedent_out.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@

def test_you(self):
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 •
edited
Loading

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.

def test_me(self):
[
item
for item in sequence
if item
]
50 changes: 35 additions & 15 deletions unittest2pytest/fixes/fix_remove_class.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,40 @@
Leaf(6, '')])])
"""

def safe_dedent(s, dedent):
"""
Dedent the prefix of a dedent token at the start of a line.

Non-syntactically meaningful newlines before tokens are appended to the prefix
of the following token, so this avoids removing the newline part of the prefix
when the token dedents to below the given level of indentation.

"""
for i, c in enumerate(s):
if c not in "\r\n":
break
else:
i = len(s)
return s[:i] + s[i:-dedent]

def dedent_suite(suite, dedent):
"""Dedent a suite in-place."""
leaves = suite.leaves()
for leaf in leaves:
if leaf.type == token.NEWLINE:
leaf = next(leaves, None)
if leaf is None:
return
if leaf.type == token.INDENT:
leaf.value = leaf.value[:-dedent]
else:
# this prefix will start with any duplicate newlines
leaf.prefix = safe_dedent(leaf.prefix, dedent)
elif leaf.type == token.INDENT:
leaf.value = leaf.value[:-dedent]
elif leaf.prefix[:1] in "\r\n":
leaf.prefix = leaf.prefix[:-dedent]

class FixRemoveClass(BaseFix):

PATTERN = """
Expand All @@ -56,25 +90,11 @@ class FixRemoveClass(BaseFix):
>
"""

def dedent(self, suite, dedent):
self.line_num = suite.get_lineno()
for kid in suite.leaves():
if kid.type in (token.INDENT, token.DEDENT):
self.line_num = kid.get_lineno()
# todo: handle tabs
kid.value = kid.value[dedent:]
self.current_indent = kid.value
elif kid.get_lineno() != self.line_num:
# todo: handle tabs
if len(kid.prefix) > len(self.current_indent):
kid.prefix = self.current_indent


def transform(self, node, results):
suite = results['suite'].clone()
# todo: handle tabs
dedent = len(find_indentation(suite)) - len(find_indentation(node))
self.dedent(suite, dedent)
dedent_suite(suite, dedent)

# remove the first newline behind the classdef header
first = suite.children[0]
Expand Down