Handle linebreaks consistently in code fixes and refactorings by amcasey · Pull Request #21158 · microsoft/TypeScript

amcasey · 2018-01-12T01:48:35Z

Don't store it on the context; do look in the EditorSettings before falling back on the LanguageServiceHost.

There's probably a bunch of renaming to do before this goes in.

NOTE: The code change is in the first commit.

mhegazy · 2018-01-12T18:32:20Z

            synchronizeHostData();
            Debug.assert(scope.type === "file");
            const sourceFile = getValidSourceFile(scope.fileName);
-            const newLineCharacter = getNewLineOrDefaultFromHost(host);


we should mark this function as deprecated, and replace all its uses as well.

It appears to be internal. Can we just delete it?

Actually, the remaining callers look pretty legitimate. Is there another method they should call instead?

removign it should be fine then.

I'm confused. This method appears to have callers. Why are we deprecating/removing it?

looking at this again, seems like in every call to this method, we have an insufficient public API. for instance, in getDocCommentTemplateAtPosition, seems like the public API should take a FormatCodeSettings just like getCodeFixesAtPosition does.
I do not think this is related to this PR per se, but this is something that we should fix for the long term. unfortunately we can not remove the getNewLineOrDefaultFromHost as you noted without breaking our public API, but we should add optional FormatCodeSettings in places where we use it to allow the host to override the value.

@mhegazy Can you please confirm that the latest commit (d97dec8) is what you had in mind (not counting a bug about reviewing callers of getNewLineOrDefaultFromHost)?

Thanks! Filed #21289.

amcasey · 2018-01-12T19:16:55Z

Name suggestions for RefactorOrCodeFixContext are welcome since it doesn't really relate to either and doesn't cover their intersection. I was thinking of something along the lines of LazyTextChangesContext, but I thought that might be confusing.

Edit: moot

amcasey · 2018-01-12T19:17:41Z

I'm also hoping there's a more idiomatic way to express the conversion from RefactorOrCodeFixContext to TextChangesContext.

Edit: moot

ghost · 2018-01-12T21:05:03Z

@@ -24,8 +24,8 @@ verify.applyCodeActionFromCompletion("b", {
    source: "/a",
    description: `Import 'foo' from module "./a"`,
    // TODO: GH#18445


Could you remove the TODOs too?

ghost · 2018-01-12T21:08:27Z

+
+    export function getNewLineFromContext(context: RefactorOrCodeFixContext) {
+        const formatSettings = context.formatContext.options;
+        return formatSettings ? formatSettings.newLineCharacter : context.host.getNewLine();


The newLineCharacter property is declared optional, so this should return context.host.getNewLine() if that's undefined.
Although it looks like getNewLine() is also optional -- see getNewLineOrDefaultFromHost.

ghost · 2018-01-12T21:10:01Z

        }

-        const changeTracker = textChanges.ChangeTracker.fromContext(context);
+        const changeTracker = textChanges.ChangeTracker.fromContext(toTextChangesContext(context));


It looks like every call to fromContext calls toTextChangesContext first -- would be easier to just do that in fromContext, and probably inline toTextChangesContext.
This would mean we don't need a TextChangesContext type, just RefactorOrCodeFixContext -- and you could rename RefactorOrCodeFixContext to TextChangesContext.

Great idea!

It's already in the EditorSettings and the LanguageServiceHost. Fixes microsoft#18291 Fixes microsoft#18445

amcasey · 2018-01-17T23:19:33Z

Straight rebase to resolve conflicts. Will address comments in subsequent commits.

@Andy-MS

Thanks to @Andy-MS for the suggestion!

ghost · 2018-01-18T00:08:02Z

+    interface SymbolContext {
        sourceFile: SourceFile;
        symbolName: string;
+        formatContext: ts.formatting.FormatContext;


Looks like this could be moved to ImportCodeFixContext.

It looks like every consumer of SymbolContext ands it with TextChangesContext anyway, so I'll just make it a subtype.

Do you know why SymbolContext exists? Is it to prevent certain functions from accessing certain members of ImportCodeFixContext?

It's just there to indicate that we don't need everything from ImportCodeFixContext.

That seems excessive to me, but not worth changing at the moment.

amcasey requested review from a user and RyanCavanaugh January 12, 2018 01:48

mhegazy reviewed Jan 12, 2018

View reviewed changes

ghost reviewed Jan 12, 2018

View reviewed changes

amcasey added 2 commits January 17, 2018 15:14

Stop explicitly storing newline in refactoring/code fix contexts

b4a382b

It's already in the EditorSettings and the LanguageServiceHost. Fixes microsoft#18291 Fixes microsoft#18445

Update test baselines

db3f7c5

amcasey force-pushed the NewlineConsistency branch from f46e0cd to db3f7c5 Compare January 17, 2018 23:19

amcasey added 3 commits January 17, 2018 15:29

Clean up TODOs for microsoft#18445

3ca1cc4

Ensure getNewLineFromContext never returns undefined

13bf7f9

Replace TextChangesContext with RefactorOrCodeFixContext

3a38c8e

Thanks to @Andy-MS for the suggestion!

ghost reviewed Jan 18, 2018

View reviewed changes

Make SymbolContext a subtype of TextChangesContext

50fd476

ghost approved these changes Jan 18, 2018

View reviewed changes

mhegazy approved these changes Jan 18, 2018

View reviewed changes

Fold newline logic into getNewLineOrDefaultFromHost

d97dec8

amcasey merged commit ebbb3a4 into microsoft:master Jan 19, 2018

amcasey deleted the NewlineConsistency branch January 19, 2018 00:02

microsoft locked and limited conversation to collaborators Jul 3, 2018

Conversation

amcasey commented Jan 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amcasey commented Jan 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amcasey commented Jan 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Jan 12, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amcasey commented Jan 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

amcasey commented Jan 12, 2018 •

edited

Loading

amcasey commented Jan 12, 2018 •

edited

Loading

ghost Jan 12, 2018 •

edited by ghost

Loading