-
-
Notifications
You must be signed in to change notification settings - Fork 187
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
feat: rewrite deprecated unittest methods #647
Conversation
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 PR tries too hard to DRY -- I would write the unittest.*
ones separately (becomes a ~4 line patch rather than a 60 line patch)
I'm not sure I understand how this patch could ever be that small. I figure that at a minimum, this needs:
Regardless, I'm happy un-DRY it. Are you looking for these replacements to be an entirely different plugin? Or an additional |
~essentially leaving everything as-is but these 4 additions: 1 new dictionary |
You don't happen to have a formatter that'll enforce the (imo, weird) indentation rules you're using, do you? I keep messing it up accidentally 😆 I'm really used to just letting I assume you'll just squash & merge, so my messy commit history is nbd, but it'd be good to know for future. |
elif ( | ||
isinstance(node.func, ast.Attribute) and | ||
isinstance(node.func.value, ast.Name) and | ||
node.func.value.id == 'unittest' and | ||
node.func.attr in FUNCTION_MAPPING | ||
): | ||
func = functools.partial( | ||
replace_name, | ||
name=node.func.attr, | ||
new=f'unittest.{FUNCTION_MAPPING[node.func.attr]}', | ||
) | ||
yield ast_to_offset(node.func), func |
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.
ahah, I'm glad we did this -- now that the code isn't muddled with DRYness it makes the subtle bug way more obvious:
these attributes are only available in a specific version of python -- right now this is performing the rewrite on all target versions
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.
these attributes are only available in a specific version of python -- right now this is performing the rewrite on all target versions
Believe it or not, I thought about that and checked ahead of time! Everything we need seem to exist in 2.7+:
someone in my chat is working on a formatter, though I don't know if it's done yet as for squash I'd prefer you'd do it because I always make merge commits but if you don't I'll "good code changed like a ghost" it for you |
Nice!
On it. |
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.
resolves #631