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 variable scope issue when renaming list comprehension variables (#293) #430

Merged
merged 23 commits into from Oct 9, 2021

Conversation

climbus
Copy link
Contributor

@climbus climbus commented Oct 6, 2021

Description

Finally, fixed issue #293

Checklist (delete if not relevant):

  • I have added tests that prove my fix is effective or that my feature works
  • I have updated CHANGELOG.md

@@ -41,138 +41,170 @@ def test_simple_global_variable_renaming(self):

def test_variable_renaming_only_in_its_scope(self):
refactored = self._local_rename(
dedent("""\
dedent(
"""\
Copy link
Member

Choose a reason for hiding this comment

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

Hi @climbus, I think these reformattings should not be made here. There is a setting in pyproject.toml that tells black that it should not be reformatting files in ropetest folder.

Can you modify your editor/black settings to make sure that it honors the pyproject.toml setting and revert these reformatting changes.

Thanks.

@climbus
Copy link
Contributor Author

climbus commented Oct 8, 2021

I was thinking why it happens to me ;)

Thanks. I fixed it.

@lieryan lieryan merged commit 030d474 into python-rope:master Oct 9, 2021
@lieryan lieryan changed the title Issue 293 Fix variable scope issue when renaming list comprehension variables (#293) Oct 9, 2021
@lieryan lieryan added this to the 0.21.x milestone Oct 9, 2021
@lieryan
Copy link
Member

lieryan commented Oct 9, 2021

Hi @climbus massive thank you and congratulations for fixing this issue.

It had been quite a complex issue to fix this.

@climbus
Copy link
Contributor Author

climbus commented Oct 9, 2021

Hi @climbus massive thank you and congratulations for fixing this issue.

It had been quite a complex issue to fix this.

No problem. I was interesting challenge ;)

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.

None yet

2 participants