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

feat: rewrite deprecated unittest methods #647

Merged
merged 1 commit into from
Jun 6, 2022
Merged

feat: rewrite deprecated unittest methods #647

merged 1 commit into from
Jun 6, 2022

Conversation

cj81499
Copy link
Contributor

@cj81499 cj81499 commented Jun 4, 2022

resolves #631

Copy link
Owner

@asottile asottile left a 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)

pyupgrade/_plugins/unittest_aliases.py Outdated Show resolved Hide resolved
@cj81499
Copy link
Contributor Author

cj81499 commented Jun 4, 2022

~4 line patch

I'm not sure I understand how this patch could ever be that small. I figure that at a minimum, this needs:

  • 3 lines for the new replacement rules
  • updates to 2 lines with hardcoded "self"

Regardless, I'm happy un-DRY it. Are you looking for these replacements to be an entirely different plugin? Or an additional dict (for unittest prefixed substitutions)? Or something else?

@asottile
Copy link
Owner

asottile commented Jun 4, 2022

~4 line patch

I'm not sure I understand how this patch could ever be that small. I figure that at a minimum, this needs:

* 3 lines for the new replacement rules

* updates to 2 lines with hardcoded `"self"`

Regardless, I'm happy un-DRY it. Are you looking for these replacements to be an entirely different plugin? Or an additional dict (for unittest prefixed substitutions)? Or something else?

~essentially leaving everything as-is but these 4 additions:

1 new dictionary
1 new elif statement below the existing one
1 partial(...) call
1 yield statement below that

@cj81499
Copy link
Contributor Author

cj81499 commented Jun 5, 2022

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 black run on save and forgetting about it entirely.

I assume you'll just squash & merge, so my messy commit history is nbd, but it'd be good to know for future.

@cj81499 cj81499 requested a review from asottile June 5, 2022 23:54
Comment on lines +67 to +79
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
Copy link
Owner

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

Copy link
Contributor Author

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+:

defaultTestLoader

loadTestsFromModule

loadTestsFromTestCase

getTestCaseNames

@asottile
Copy link
Owner

asottile commented Jun 6, 2022

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

@cj81499
Copy link
Contributor Author

cj81499 commented Jun 6, 2022

someone in my chat is working on a formatter, though I don't know if it's done yet

Nice!

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

On it.

@cj81499 cj81499 requested a review from asottile June 6, 2022 19:21
Copy link
Owner

@asottile asottile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asottile asottile merged commit ef3be8c into asottile:main Jun 6, 2022
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.

Feature Proposal: Rewrite unittest.makeSuite|getTestCaseNames|findTestCases.
2 participants