moveToNewFile: Infer quote preference#24652
moveToNewFile: Infer quote preference#246521 commit merged intomastermicrosoft/TypeScript:masterfrom moveToNewFile_inferQuoteStylemicrosoft/TypeScript:moveToNewFile_inferQuoteStyleCopy head branch name to clipboard
Conversation
| if (moduleExportsChangedToDefault) { | ||
| for (const importingFile of program.getSourceFiles()) { | ||
| fixImportOfModuleExports(importingFile, sourceFile, changes, preferences); | ||
| fixImportOfModuleExports(importingFile, sourceFile, changes, getQuotePreference(importingFile, preferences)); |
There was a problem hiding this comment.
what about the other preferences? why aren't we pass the other ones too?
There was a problem hiding this comment.
We're currently not using any other preferences; if we did we would need to create a ProcessedPreferences interface since we don't want to pass raw preferences, we want preferences with quote style inferred.
There was a problem hiding this comment.
so shouldn't it be :
{...preferences, quotePreference: getQuotePreference(prefrences)} There was a problem hiding this comment.
quotePreference in UserPreferences is a different type than QuotePreference -- and it should be, because we want the type checker to ensure that we've processed the preferences.
There was a problem hiding this comment.
but we are leaving behind all other preferences.. the next user preference we add will not be passed through.
There was a problem hiding this comment.
That's true, which means we would have to change the code to pass more parameters if that turned out to be necessary. Or I could create ProcessedPreferences interface right now if you prefer?
| return createLiteral(text, quotePreference === QuotePreference.Single); | ||
| } | ||
|
|
||
| export const enum QuotePreference { Single, Double } |
There was a problem hiding this comment.
VS Code also lists auto as an option. I asked @mhegazy about it and there seemed to be some confusion about how to distinguish between "figure it out for me" and "keep it in its current state". I don't think sorting that out will require changes to this code, but it's something to keep in mind.
| /// <reference path='fourslash.ts' /> | ||
|
|
||
| // @Filename: /a.ts | ||
| ////import 'unrelated'; |
There was a problem hiding this comment.
Naively, I would have expected to see a test for each possible inference (i.e. one single, one double).
There was a problem hiding this comment.
Most other moveToNewFile tests are using double quotes, since that's the default inference.
|
Please port to release-2.9 as well. |
Fixes #24605