FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add code fix to remove unused label · Pull Request #24037 · microsoft/TypeScript · GitHub

/ TypeScript Public

Add code fix to remove unused label - #24037

Merged
3 commits merged into
masterfrom
fixUnusedLabel
May 18, 2018
ConversationCommits (3)ChecksFiles changed
Merged

Add code fix to remove unused label#24037
3 commits merged into
masterfrom
fixUnusedLabel

Conversation

Deleted user (ghost) commented May 10, 2018

Copy link
Copy Markdown

Similar to #24028

ghost requested a review from Andrew Casey (amcasey) May 10, 2018 21:31
Mohamed Hegazy (mhegazy) reviewed May 10, 2018
View reviewed changes
Comment thread src/services/codefixes/fixUnusedLabel.ts Outdated
function doChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, start: number): void {
const token = getTokenAtPosition(sourceFile, start, /*includeJsDocComment*/ false);
const statement = cast(token.parent, isLabeledStatement).statement;
changes.deleteRange(sourceFile, { pos: token.getStart(sourceFile), end: statement.getStart(sourceFile) });

Mohamed Hegazy (mhegazy) May 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

this will remove comments on the statement before the label.. if you are deleting a range, why not just delete from token.pos to :.end?

Deleted user (ghost) May 10, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I wanted to remove the space between the : and the statement following it. token.getStart should not include any comments preceding token. (But token.pos would.)

Andrew Casey (amcasey) May 10, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

/*not deleted*/ label /*deleted*/ : /*deleted*/ call();?

Andrew Casey (amcasey) approved these changes May 10, 2018
View reviewed changes

Andrew Casey (amcasey) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I have concerns about the trivia, but I understand you're tracking that as its own (increasingly expensive) work item. Otherwise, LGTM.

Comment thread src/services/codefixes/fixUnusedLabel.ts Outdated
function doChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, start: number): void {
const token = getTokenAtPosition(sourceFile, start, /*includeJsDocComment*/ false);
const statement = cast(token.parent, isLabeledStatement).statement;
changes.deleteRange(sourceFile, { pos: token.getStart(sourceFile), end: statement.getStart(sourceFile) });

Andrew Casey (amcasey) May 10, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

/*not deleted*/ label /*deleted*/ : /*deleted*/ call();?

Comment thread tests/cases/fourslash/codeFixUnusedLabel_all.ts

// @noUnusedLocals: true

////label1: while (1) {}

Andrew Casey (amcasey) May 10, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What happens for outdented labels on preceding lines?

label:
    code

Does formatting restore the indentation? Otherwise, it seems like you'd end up with code outdented.

Deleted user (ghost) May 18, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fixed.

ghost force-pushed the fixUnusedLabel branch from 3a501cc to 3e1f013 Compare May 18, 2018 21:43
ghost force-pushed the fixUnusedLabel branch from 3e1f013 to 76f56a5 Compare May 18, 2018 22:05
ghost merged commit 3eb66da into master May 18, 2018
ghost deleted the fixUnusedLabel branch May 18, 2018 22:25
Microsoft (microsoft) locked and limited conversation to collaborators Jul 31, 2018
This pull request was closed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL