Add support for error templates in tests #2481
Conversation
ajafff
changed the title
|
Is it common to manually generate the .lint file errors? My workflow is usually to write out the file without the errors, run the test, then copy the generated actual output. This feature seems unnecessarily complex. |
At least I do. And given that @andy-hanson uses message substitutions a lot in his PRs, I think he also does. Call it test driven development if you like. I want to test what I expect the implementation to do instead of using the output of the initial implementation for future testing. |
src/test/parse.ts
Outdated
| @@ -114,6 +118,25 @@ export function parseErrorsFromMarkup(text: string): LintError[] { | |||
| return lintErrors; | |||
| } | |||
|
|
|||
| function substituteMessage(substitutions: Map<string, string>, message: string): string { | |||
There was a problem hiding this comment.
can you rename substitutions to templateMap? It's confusing because the values that are put into the template are also substitutions. Maybe also rename message to something like templateCall
There was a problem hiding this comment.
Re substitutions: It's not really a templateMap, it's a map of substitutions that may happen to contain a template
Re message: It's not necessarily a templateCall. It may be an identifier of a substitution, a "template call" or just plain text
I'd prefer leaving the names that way.
src/test/parse.ts
Outdated
| } | ||
|
|
||
| function formatMessage(substitutions: Map<string, string>, message: string): string { | ||
| const formatMatch = /^([\w_]+) % \(\s*([^,]+(?:\s*,\s*[^,]+)*)\s*\)$/.exec(message); |
There was a problem hiding this comment.
add a comment somewhere about the expected format of message and the substitutions since the regex is hard to parse
src/test/parse.ts
Outdated
| if (formatMatch !== null) { | ||
| const base = substitutions.get(formatMatch[1]); | ||
| if (base !== undefined) { | ||
| message = format(base, ...(formatMatch[2].trim().split(/\s*,\s*/g))); |
There was a problem hiding this comment.
what if the argument has a comma or space? Spaces could be trimmed away and commas could cause unwanted splits. Consider using quotes around each arg
There was a problem hiding this comment.
Format arguments now need quotes. The implementation uses typescript's scanner to parse them. Escaping also works.
|
also needs docs. please update https://github.com/palantir/tslint/blob/master/docs/develop/testing-rules/index.md |
|
@nchen63 I updated the implementation and added docs |
src/test/parse.ts
Outdated
| * Where `name` is the name of a message substitution that is used as format string. | ||
| * If `name` is not found in `substitutions`, `message` is returned unchanged. | ||
| */ | ||
| function formatMessage(substitutions: Map<string, string>, message: string): string { |
There was a problem hiding this comment.
The reason I suggested the name change, is that if one were to look at just this function signature, you'd assume that substitutions: Map<string, string> is a map between placeholders and replacement values, and that message is the template string.
Also, I think it's fine calling these templates since template strings don't necessarily have placeholders
There was a problem hiding this comment.
That makes sense. I renamed it to templates hoping it's less confusing now
Add the ability to use placeholders in message substitutions. Syntax is inspired by python. It uses node's `util.format()` under the hood.
|
@nchen63 @adidahiya you both approved this PR. Can this be merged? I recently missed this feature while I refactored some rules and needed to edit their tests. |
|
@ajafff thanks for the reminder, sorry for the delay. |
PR checklist
Overview of change:
It's pretty unwieldy to write and maintain test for rules that contain a name in the error message (e.g.
prefer-const,no-unused-variable,no-shadowed-variable, ...). I added basic template support to error message substitutions which uses node'sutil.format()to replace placeholders.Syntax is inspired by python:
name % (substitution1[, substitution2, ...]). I doubt this will accidentally break anyone's tests.The added test shows best what can be done with this.
As an example I also rewrote the tests of
prefer-constto use templates.Is there anything you'd like reviewers to focus on?
Do you have a better idea for the syntax?
I'll add documentation once there is consensus regarding the syntax.
CHANGELOG.md entry:
[develop] added support for error templates in rule tests