feat!: Rule Tester checks for missing placeholder data in the message #18073
Conversation
|
Hi @DMartens!, thanks for the Pull Request The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.
To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page. Read more about contributing to ESLint here |
✅ Deploy Preview for docs-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
DMartens
changed the title
lib/rule-tester/rule-tester.js
Outdated
| @@ -304,6 +304,17 @@ function throwForbiddenMethodError(methodName, prototype) { | |||
| }; | |||
| } | |||
|
|
|||
| const messagePlaceholderRegex = /\{\{([^{}]+?)\}\}/gu; | |||
There was a problem hiding this comment.
I do think this regexp should be exported by a shared util file so it can be used in multiple places.
|
Implemented all the suggestions except for the potential exporting of the regular expression.
Also now that the missing placeholders are also checked when |
I think this would be fine.
|
I think interpolate shouldn't be multipass/recursive, and the test that fails indeed expects the data text to appear in its raw form in the final message. |
tests/lib/rule-tester/rule-tester.js
Outdated
| valid: [], | ||
| invalid: [{ code: "foo", errors: [{ messageId: "avoidFoo" }] }] | ||
| }); | ||
| }, "The reported message has a missing placeholder 'placeholder'. Please provide them via the 'data' property in the context.report() call."); |
There was a problem hiding this comment.
This error message seems misleading since, as @mdjermanovic said, we don't want to suggest that recursive/multipass interpolation is allowed. If there was some way to escape placeholders in data, that could solve it.
There was a problem hiding this comment.
When only messageId is specified in the test case, I think we could just check if the meta message really has those placeholders we found as missing. While theoretically the same name can appear as a placeholder and in data, that seems like an extreme edge case.
If there is test If there isn't test |
lib/rule-tester/rule-tester.js
Outdated
| const existing = getMessagePlaceholders(message); | ||
|
|
||
| if (data === void 0) { | ||
| const available = getMessagePlaceholders(raw); | ||
|
|
||
| return existing.filter(name => available.includes(name)); | ||
| } | ||
|
|
||
| const introduced = new Set(Object.values(data).flatMap(value => (typeof value === "string" ? getMessagePlaceholders(value) : []))); | ||
|
|
||
| return existing.filter(name => !introduced.has(name)); |
There was a problem hiding this comment.
| const existing = getMessagePlaceholders(message); | |
| if (data === void 0) { | |
| const available = getMessagePlaceholders(raw); | |
| return existing.filter(name => available.includes(name)); | |
| } | |
| const introduced = new Set(Object.values(data).flatMap(value => (typeof value === "string" ? getMessagePlaceholders(value) : []))); | |
| return existing.filter(name => !introduced.has(name)); | |
| let existing = getMessagePlaceholders(message); | |
| // no potential placeholders were found in the reported message, so there's nothing further to check | |
| if (existing.length === 0) { | |
| return existing; | |
| } | |
| const original = getMessagePlaceholders(raw); | |
| // filter out those that don't appear in the original template message as they're certainly not placeholders | |
| existing = existing.filter(name => original.includes(name)); | |
| if (data) { | |
| const provided = Object.keys(data); | |
| // since the hydrated message texts are compared, we can ignore those for which the data is provided in the test | |
| existing = existing.filter(name => !provided.includes(name)); | |
| } | |
| return existing; |
I think this check would be more robust if we always exclude those that don't appear in the meta message and those for which the data is provided in the test.
There was a problem hiding this comment.
I refactored this method in another way and also kept the case for adding placeholders via data properties (see no-restricted-syntax case).
lib/rule-tester/rule-tester.js
Outdated
|
|
||
| assert.ok( | ||
| missingPlaceholders.length === 0, | ||
| `The reported message has ${missingPlaceholders.length > 1 ? `the missing placeholders: ${missingPlaceholders.map(name => `'${name}'`).join(", ")}` : `a missing placeholder '${missingPlaceholders[0]}'`}. Please provide them via the 'data' property in the context.report() call.` |
There was a problem hiding this comment.
| `The reported message has ${missingPlaceholders.length > 1 ? `the missing placeholders: ${missingPlaceholders.map(name => `'${name}'`).join(", ")}` : `a missing placeholder '${missingPlaceholders[0]}'`}. Please provide them via the 'data' property in the context.report() call.` | |
| `The reported message has ${missingPlaceholders.length > 1 ? `unsubstituted placeholders: ${missingPlaceholders.map(name => `'${name}'`).join(", ")}` : `an unsubstituted placeholder '${missingPlaceholders[0]}'`}. Please provide them via the 'data' property in the context.report() call.` |
I think that "missing placeholders" sounds like the data was provided but the corresponding placeholders were not found in the message template, while this is the opposite case - placeholders are in the template but the data for them was not provided. Maybe "missing" -> "unsubstituted"?
There was a problem hiding this comment.
I used the term missing as this is the cause of the issue. That the placeholder cannot be substituted is the result of that. But I also don't really like missing and I cannot come up with a better name for it, so I changed it in the messages and variable names.
There was a problem hiding this comment.
I think this is a grammar issue: the placeholder isn't missing, it's the value to use for the placeholder that's missing. Another option would be "missing placeholder value".
|
Also, there are merge conflicts in |
|
@DMartens can you fix merge conflicts? We'd like to include this in today's v9.0.0-beta.0 release. |
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
DMartens
force-pushed
the
rule-tester-check-missing-placeholder-data
branch
from
7378079
to
09420cf
Compare
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
DMartens
force-pushed
the
rule-tester-check-missing-placeholder-data
branch
from
09420cf
to
22ee891
Compare
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
mdjermanovic
added
core
fixture for non-string data values
|
I've verified that all review comments have been addressed, so merging for the release. |
mdjermanovic
changed the title
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain: Rule Tester
What changes did you make? (Give an overview)
Fixes #18016.
Added a check for errors and suggestions which provide a
messageIdbut not specify adataproperty.This check makes sure that the replaced message has no placeholders as the required
dataproperties were provided.Is there anything you'd like reviewers to focus on?
interpolate.js; maybe the regular expression should be exported?dataproperty in the error. Maybe this check should run before as currently thedatacheck does two things: detect missing placeholder data and checks whether the values matchdatais not shared with the top leveldata?