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

Change default newLine, forceConsistentCasingInFileNames #52298

Merged
merged 20 commits into from
Jan 21, 2023

Conversation

RyanCavanaugh
Copy link
Member

Per discussion at #52296

* Default newline is now lf instead of os-specific
* forceConsistentCasingInFilenames now defaults to true
@RyanCavanaugh RyanCavanaugh added the Breaking Change Would introduce errors in existing code label Jan 19, 2023
@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jan 19, 2023
Comment on lines 1069 to 1068
const newLineChar = getNewLineCharacter(options, maybeBind(host, host.getNewLine));
const newLineChar = getNewLineCharacter(options);
Copy link
Member

Choose a reason for hiding this comment

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

Just to double check, this won't cause our completion snippets to have the wrong newlines, right? This is sort of awkward because it's the input newline character we'd want in snippets, not the output type. I'm not sure if host.getNewLine here was overriding that choice or what, but I guess there's no way this code could have determined the "right" thing to use without using the SourceFile, so, I'm confused.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, the current errors in baselines seem to imply that it does matter?

@jakebailey
Copy link
Member

jakebailey commented Jan 20, 2023

After this merges, I think I'll send a PR which tries to add it to .git-blame-ignore-revs and see if it works; it would be convenient at least.

Copy link
Member

@DanielRosenwasser DanielRosenwasser left a comment

Choose a reason for hiding this comment

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

As Jake mentioned, this is changing up the source-authored newlines used for edits in the LS with the output newlines. Am I missing something or is that intentional?

@DanielRosenwasser
Copy link
Member

Probably need to update getNewLineOrDefaultFromHost.

@jakebailey
Copy link
Member

I'm guessing that function is what those snippet printers are supposed to use.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jan 20, 2023

Yes, I would assume most instances of getNewLineCharacter in completions.ts should be replaced with (or paired with (unlikely?)) getNewLineOrDefaultFromHost.

@jakebailey
Copy link
Member

I'm also guessing nobody's noticed because VS Code normalizes them to the desired endings?

@RyanCavanaugh
Copy link
Member Author

I'm going to try a reduced subset of this PR to avoid the LS implications, which we don't really want to touch

@RyanCavanaugh
Copy link
Member Author

Pared down at #52336

@RyanCavanaugh
Copy link
Member Author

OK, I think this is good to go. I've manually tested both settings as well

@@ -27,7 +27,21 @@ Output::
>> Screen clear
[12:00:15 AM] Starting compilation in watch mode...

[12:00:20 AM] Found 0 errors. Watching for file changes.
b.ts:1:43 - error TS1149: File name '/A.ts' differs from already included file name '/a.ts' only in casing.
Copy link
Member

Choose a reason for hiding this comment

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

whats this diff?

Copy link
Member

Choose a reason for hiding this comment

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

Oh because default changed. can you change the test to set forceConsistenCasing to true and false instead of using default so it is actually checking what the test is suppose to check.

@@ -27,7 +27,21 @@ Output::
>> Screen clear
[12:00:15 AM] Starting compilation in watch mode...

[12:00:20 AM] Found 0 errors. Watching for file changes.
b.ts:1:43 - error TS1149: File name '/A.ts' differs from already included file name '/a.ts' only in casing.
Copy link
Member

Choose a reason for hiding this comment

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

Oh because default changed. can you change the test to set forceConsistenCasing to true and false instead of using default so it is actually checking what the test is suppose to check.

@@ -2437,7 +2437,7 @@ export function createLanguageService(
}

function getDocCommentTemplateAtPosition(fileName: string, position: number, options?: DocCommentTemplateOptions): TextInsertion | undefined {
return JsDoc.getDocCommentTemplateAtPosition(getNewLineOrDefaultFromHost(host), syntaxTreeCache.getCurrentSourceFile(fileName), position, options);
return JsDoc.getDocCommentTemplateAtPosition(getNewLineOrDefaultFromHost(host, /*formatSettings*/ undefined), syntaxTreeCache.getCurrentSourceFile(fileName), position, options);
Copy link
Member

Choose a reason for hiding this comment

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

Not that you should fix it in this PR, but it seems like a bug that we don't have this for JSDoc comments.

@DanielRosenwasser
Copy link
Member

@typescript-bot user test tsserver
@typescript-bot test tsserver top100

Just for fun.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 21, 2023

Heya @DanielRosenwasser, I've started to run the diff-based user code test suite (tsserver) on this PR at d40ab04. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 21, 2023

Heya @DanielRosenwasser, I've started to run the diff-based top-repos suite (tsserver) on this PR at d40ab04. You can monitor the build here.

Update: The results are in!

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

LGTM (again); I'm pretty sure those last two remaining /*formatSettings*/ undefined are preexisting bugs we should report somewhere.

@typescript-bot
Copy link
Collaborator

@DanielRosenwasser Here are the results of running the user test suite comparing main and refs/pull/52298/merge:

Everything looks good!

@@ -781,7 +781,7 @@ class LanguageServiceShimObject extends ShimBase implements LanguageServiceShim
}

private realizeDiagnostics(diagnostics: readonly Diagnostic[]): { message: string; start: number; length: number; category: string; }[] {
const newLine = getNewLineOrDefaultFromHost(this.host);
const newLine = getNewLineOrDefaultFromHost(this.host, /*formatSettings*/ undefined);
Copy link
Member

Choose a reason for hiding this comment

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

I think this one is probably okay.

@DanielRosenwasser DanielRosenwasser merged commit ff92ab0 into microsoft:main Jan 21, 2023
@typescript-bot
Copy link
Collaborator

@DanielRosenwasser Here are the results of running the top-repos suite comparing main and refs/pull/52298/merge:

Everything looks good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team Breaking Change Would introduce errors in existing code For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants