GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Add code fix to remove unused label · Pull Request #24037 · microsoft/TypeScript

Conversation

@ghost

@ghost Deleted user (ghost) commented May 10, 2018

Copy link
Copy Markdown

Similar to #24028

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Andy Hanson
Add code fix to remove unused label
23d7763
@ghost
ghost requested a review from Andrew Casey (amcasey) May 10, 2018 21:31
mhegazy
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) });

@mhegazy Mohamed Hegazy (mhegazy) May 10, 2018

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ghost Deleted user (ghost) May 10, 2018

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@amcasey Andrew Casey (amcasey) May 10, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
amcasey
Andrew Casey (amcasey) approved these changes May 10, 2018
View reviewed changes

@amcasey Andrew Casey (amcasey) left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
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) });

@amcasey Andrew Casey (amcasey) May 10, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread tests/cases/fourslash/codeFixUnusedLabel_all.ts

// @noUnusedLocals: true

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

@amcasey Andrew Casey (amcasey) May 10, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ghost Deleted user (ghost) May 18, 2018

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
mhegazy
Mohamed Hegazy (mhegazy) approved these changes May 10, 2018
View reviewed changes
Andy Hanson
Test with trivia and fix indentation with dedented label
ebddceb
@ghost
ghost force-pushed the fixUnusedLabel branch from 3a501cc to 3e1f013 Compare May 18, 2018 21:43
Andy Hanson
Merge branch 'master' into 'fixUnusedLabel'
76f56a5
@ghost
ghost force-pushed the fixUnusedLabel branch from 3e1f013 to 76f56a5 Compare May 18, 2018 22:05
@ghost
ghost merged commit 3eb66da into master May 18, 2018
@ghost
ghost deleted the fixUnusedLabel branch May 18, 2018 22:25
@microsoft Microsoft (microsoft) locked and limited conversation to collaborators Jul 31, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@mhegazy @amcasey
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.