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

Sort extract constant refactoring before extract function #29589

Closed
mjbvz opened this issue Jan 25, 2019 · 3 comments · Fixed by #35580
Closed

Sort extract constant refactoring before extract function #29589

mjbvz opened this issue Jan 25, 2019 · 3 comments · Fixed by #35580
Assignees
Labels
Domain: Refactorings e.g. extract to constant or function, rename symbol Domain: TSServer Issues related to the TSServer Fixed A PR has been merged for this issue In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@mjbvz
Copy link
Contributor

mjbvz commented Jan 25, 2019

Search terms

  • Refactor, refactorings
  • extract

Feature request
When both extract const and extract function refactorings are available for a piece of code, show the extract const before extract function

VS Code shows the refactorings in the order that TS returns them to us in.

Related to #29587

@weswigham weswigham added Domain: TSServer Issues related to the TSServer Domain: Refactorings e.g. extract to constant or function, rename symbol Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Jan 29, 2019
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 3.8.0 milestone Oct 25, 2019
@andrewbranch andrewbranch added the Fix Available A PR has been opened for this issue label Dec 9, 2019
@DanielRosenwasser DanielRosenwasser added Fixed A PR has been merged for this issue and removed Fix Available A PR has been opened for this issue labels Dec 9, 2019
@markusjohnsson
Copy link
Contributor

Woah, is this at all based on analytics, or just a hunch from a couple of reporters? I’m not sure, as I use both, but I think I “extract to function” a more often.

@andrewbranch
Copy link
Member

I can’t speak for @mjbvz or VS Code, but keep in mind that “extract to function” is available far more often than “extract to constant.” Only single expressions can be extracted to constants, while anything from a single expression to multiple consecutive statements can be extracted to a function. I suspect the reasoning is that it would be rare for the entire body of a function to be a return statement of a single expression (e.g. function getDuration() { return 60 * 1000; }), “extract to constant” probably makes more sense whenever it’s available (e.g. const duration = 60 * 1000).

Hopefully there were analytics backing this, but I simply mean to caution that using “extract to function” more often than “extract to constant” isn’t a good proxy measure, since they’re not always coincident.

@markusjohnsson
Copy link
Contributor

Thanks @andrewbranch , of course, I didn’t think of that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: Refactorings e.g. extract to constant or function, rename symbol Domain: TSServer Issues related to the TSServer Fixed A PR has been merged for this issue In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants