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.

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

Jump to bottom

Removing an import alias with a trailing standalone comment should preserve the comment #392

Merged
merged 3 commits into from
Sep 24, 2020
Merged

Removing an import alias with a trailing standalone comment should preserve the comment #392

merged 3 commits into from
Sep 24, 2020

Conversation

zsol
Copy link
Member

@zsol zsol commented Sep 23, 2020 •
edited
Loading

Summary

When removing import aliases, comments often get mangled (see test cases for details). This PR fixes comment handling.

Test Plan

Added test case

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 23, 2020
@zsol zsol marked this pull request as ready for review September 23, 2020 17:46
@zsol zsol requested a review from jimmylai September 24, 2020 08:45
jimmylai
jimmylai approved these changes Sep 24, 2020
View reviewed changes
Copy link
Contributor

@jimmylai jimmylai left a comment

Choose a reason for hiding this comment

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

LGTM

Some of the logic can probably be built as reusable helpers. Especially moving the comments in ParenthesizedWhitespace is also pretty common in other codemod use case like remove an argument from a call.

@jimmylai jimmylai merged commit 0a4d1e4 into Instagram:master Sep 24, 2020
@zsol zsol mentioned this pull request Sep 29, 2020
Merged
zsol added a commit that referenced this pull request Oct 1, 2020
@zsol
fix RemoveImportsVisitor crash when ImportAlias is inserted without c…
1d22a29 
…omma (#397)

The comment preserving logic introduced in #392 assumed that in an ImportFrom node, ImportAliases have a comma property (except for the last one). That's only true if the ImportFrom node is parsed from actual source, but isn't necessarily true if it's constructed manually.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimmylai jimmylai jimmylai approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zsol @jimmylai @facebook-github-bot