Skip to content

moveToNewFile: handle namespace imports too #38377

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
Merged
merged 1 commit into from
May 7, 2020
Merged

moveToNewFile: handle namespace imports too #38377

merged 1 commit into from
May 7, 2020

Conversation

elibarzilay
Copy link
Contributor

Fixes #35591

elibarzilay
elibarzilay commented May 6, 2020
View reviewed changes
src/services/refactors/moveToNewFile.ts
@@ -533,6 +533,7 @@ namespace ts.refactor {
case SyntaxKind.ImportEqualsDeclaration:
case SyntaxKind.ImportSpecifier:
case SyntaxKind.ImportClause:
case SyntaxKind.NamespaceImport:
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 thought about adding a comment given that NamespaceImport is in this list but NamedImports, but I'm half-guessing that it would be semi-obvious for most people, and I don't know if there's any reference for the list of syntax kinds.

Copy link
Member

Choose a reason for hiding this comment

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

I actually don't know why this is - is it because it's somehow handled by ImportClause?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DanielRosenwasser, my guess is that one of NamedImports or ImportClause is for the whole import and the other is for each name in the import, or something similar (since the first is plural). But I don't want to add a comment with a guess... I did see that adding NamedImports doesn't break any tests, but went with the smaller change.

Copy link
Member

@DanielRosenwasser DanielRosenwasser May 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

That decision makes sense, thanks!

@RyanCavanaugh
Copy link
Member

@DanielRosenwasser appetite for taking this to 3.9 final instead of waiting for a servicing release, given the diff? Will queue up a port PR either way

@RyanCavanaugh
Copy link
Member

@typescript-bot cherry-pick this to release-3.9

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 7, 2020 •
edited
Loading

Heya @RyanCavanaugh, I've started to run the task to cherry-pick this into release-3.9 on this PR at 4baff1b. You can monitor the build here.

@typescript-bot typescript-bot mentioned this pull request May 7, 2020
Merged
@typescript-bot
Copy link
Collaborator

Hey @RyanCavanaugh, I've opened #38386 for you.

typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request May 7, 2020
@elibarzilay @typescript-bot
Cherry-pick PR microsoft#38377 into release-3.9
f5d878c 
Component commits:
4baff1b moveToNewFile: handle namespace imports too
Fixes microsoft#35591
@DanielRosenwasser
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 7, 2020 •
edited
Loading

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 4baff1b. You can monitor the build here.

@DanielRosenwasser
Copy link
Member

Looks reasonable for 3.9.2. I'll merge it into release-3.9 tomorrow once this goes in.

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 7, 2020 •
edited
Loading

Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/73189/artifacts?artifactName=tgz&fileId=250DEC821D448288B493C71ED8BCF23627C53113160C6F2993FC90513F3162AD02&fileName=/typescript-4.0.0-insiders.20200507.tgz"
    }
}

and then running npm install.

There is also a playground for this build.

@elibarzilay elibarzilay merged commit 29cf049 into microsoft:master May 7, 2020
@elibarzilay elibarzilay deleted the 35591 branch May 7, 2020 16:03
DanielRosenwasser pushed a commit that referenced this pull request May 8, 2020
@elibarzilay
Cherry-pick PR #38377 into release-3.9 (#38386)
ef028bc 
Component commits:
4baff1b moveToNewFile: handle namespace imports too
Fixes #35591

Co-authored-by: Eli Barzilay <eli@barzilay.org>
@jakebailey jakebailey mentioned this pull request Jul 21, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanielRosenwasser DanielRosenwasser DanielRosenwasser approved these changes

@RyanCavanaugh RyanCavanaugh RyanCavanaugh approved these changes

Assignees

@elibarzilay elibarzilay

Labels
Author: Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move to file does not include namespace imports
4 participants
@elibarzilay @RyanCavanaugh @typescript-bot @DanielRosenwasser