Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Jul 20, 2017

Tested with:

diff TypeScript/src/compiler/diagnosticInformationMap.generated.ts TypeScript_1/TypeScript/src/compiler/diagnosticInformationMap.generated.ts
diff TypeScript/src/compiler/diagnosticMessages.generated.json TypeScript_1/TypeScript/src/compiler/diagnosticMessages.generated.json

No change.

@ghost ghost force-pushed the processDiagnosticMessages branch from ac962b8 to bbc9d65 Compare July 20, 2017 21:42
@ghost
Copy link
Author

ghost commented Jul 20, 2017

@weswigham Any idea why gulp generate-diagnostics doesn't seem to run processDiagnosticMessages.ts? I put if (1 + 1 === 2) throw new Error("!"); at the top and it still succeeded.

@weswigham
Copy link
Member

weswigham commented Jul 25, 2017

@Andy-MS if (needsUpdate(diagnosticMessagesJson, [generatedDiagnosticMessagesJSON, diagnosticInfoMapTs])) is checking if the generatedJSON and info map TS file are older than the input json file. Editing the actual generation script won't update the input json, so it won't be run. Add the generating ts or js file to the list of files to compare to to get it to consider it for change-time-tracking.

@ghost
Copy link
Author

ghost commented Jul 25, 2017

OK, I had been confused before because in the old implementation, isCaseSensitive was hard-coded to false. But we actually had the condition on backwards:

if (caseSensitive) {
    s1 = s1.toLowerCase();
    s2 = s2.toLowerCase();
}

Since isCaseSensitive was always false, stringEquals was just a == test.

@ghost ghost requested review from aozgaa and weswigham July 25, 2017 20:08
Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

This removes the automatic generation of noncolliding names and introduces case sensitivity. I think the case sensitivity is probably bad - we probably shouldn't be introducing multiple diagnostics which only differ in case (even if that was the old behavior), and it's good to catch that here. The automatic collision renaming... well, if we're not triggering it then I guess it doesn't really have a point and is fine to remove.

@ghost
Copy link
Author

ghost commented Jul 27, 2017

@weswigham We already have both Diagnostics.file and Diagnostics.FILE. In one case we want this word to appear in lowercase, in another we want it uppercase. I don't see any problem with that?
This PR does not introduce case sensitivity, it just makes it obvious that that was always the way things were done.

@weswigham
Copy link
Member

Alright.

@ghost ghost merged commit 12acc14 into master Jul 27, 2017
@ghost ghost deleted the processDiagnosticMessages branch July 27, 2017 19:30
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants