Improve non-ambient class and function merge error#33441
Improve non-ambient class and function merge error#33441austincummings wants to merge 1 commit intomicrosoft:masterfrom austincummings:fix32795
Conversation
| : undefined; | ||
| if (diagnostic) { | ||
| addRelatedInfo( | ||
| error(getNameOfDeclaration(declaration) || declaration, diagnostic, symbolName(symbol)), |
There was a problem hiding this comment.
Is it safe to provide symbolName(symbol) even if the diagnostic doesn't specify a parameter?
There was a problem hiding this comment.
Yep, from reading the code, it looks for the params in the string and matches the values to that - basically it'll just not do anything 👍
export function formatStringFromArgs(text: string, args: ArrayLike<string | number>, baseIndex = 0): string {
return text.replace(/{(\d+)}/g, (_match, index: string) => "" + Debug.assertDefined(args[+index + baseIndex]));
}
| "category": "Error", | ||
| "code": 2774 | ||
| }, | ||
| "Function with bodies can only merge with classes that are ambient.": { |
There was a problem hiding this comment.
Is "bodies" right? Or can there only be one function body?
There was a problem hiding this comment.
Good point, there's only one body, I think we should probably make these two error messages:
Class declaration cannot implement overload list for '{0}'.Functions with bodies can only merge with classes that are ambient.
orta
left a comment
orta
left a comment
There was a problem hiding this comment.
Looking good! Only some minor wording changes 👍
| : undefined; | ||
| if (diagnostic) { | ||
| addRelatedInfo( | ||
| error(getNameOfDeclaration(declaration) || declaration, diagnostic, symbolName(symbol)), |
There was a problem hiding this comment.
Yep, from reading the code, it looks for the params in the string and matches the values to that - basically it'll just not do anything 👍
export function formatStringFromArgs(text: string, args: ArrayLike<string | number>, baseIndex = 0): string {
return text.replace(/{(\d+)}/g, (_match, index: string) => "" + Debug.assertDefined(args[+index + baseIndex]));
}| "category": "Error", | ||
| "code": 2774 | ||
| }, | ||
| "Function with bodies can only merge with classes that are ambient.": { |
There was a problem hiding this comment.
Good point, there's only one body, I think we should probably make these two error messages:
Class declaration cannot implement overload list for '{0}'.Functions with bodies can only merge with classes that are ambient.
|
Poke ^ |
|
Hi @austincummings - this PR didn't have 'allow maintainers to make changes' checked, and so when we migrated TypeScript's primary branch from 'master' to 'main' this PR couldn't have its branch re-targeted. It looks like most of my changes were cosmetics, so once I've got my current PR up and running, I'll replicate this PR with my changes included and get it reviewed 👍🏻 |
3 participants
Fixes #32795