feat(eslint-plugin): add options for custom ID and default value validation #2186
Conversation
…dation Added two more options to report errors if: - Custom ID is not present - Custom ID is present but it doesn't follow the allowed pattern - Default value is not provided Change in details: - Added new configuration options and error messages for reporting. - Create and use a `validate` function that receives the input string and a type to validate optionally it can receive a pattern to validate custom id. - The core of the `validate` function is a regular expression with named group, I have followed how Angular manages metadata when using `$localize` - Added new valid and invalid tests cases to cover the changes. New config after changes: - Added `requireCustomId` - `boolean` - default is `false` - Added `requireDefaultValue` - `boolean` - default is `false` - Added `boundTextAllowedPattern` - `string` - No default value. It expects to get a string that can be converted to regular expression to test custom id. I have used the same name that already exists in `@angular-eslint/template-parser`.
|
Any update on this PR? |
packages/eslint-plugin/package.json
Outdated
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "@angular-eslint/eslint-plugin", | |||
| "version": "19.0.2", | |||
| "version": "19.1.0", | |||
There was a problem hiding this comment.
Revert this, versioning is automated and does not happen through imperative updates to version fields, and all packages are released together so changing only one package would never be the right thing to do.
| } | ||
| } | ||
| return result; | ||
| }; | ||
|
|
||
| const STYLE_GUIDE_LINK = 'https://angular.dev/guide/i18n'; | ||
| const STYLE_GUIDE_LINK_COMMON_PREPARE = `${STYLE_GUIDE_LINK}-common-prepare`; |
There was a problem hiding this comment.
This link no longer seems to be correct at all (I think the new one is https://angular.dev/guide/i18n/prepare?) so let's please fix it as part of this PR so that all the various errors have reasonable links.
I discovered this when trying to verify the ones you added (which should be here now I believe https://angular.dev/guide/i18n/manage-marked-text)
| type: 'boolean', | ||
| default: DEFAULT_OPTIONS.requireDefaultValue, | ||
| }, | ||
| boundTextAllowedPattern: { |
There was a problem hiding this comment.
I don't think this is very clear that it relates to custom ID.
I think something like this would be much clearer:
{
"@angular-eslint/require-localizae-metadata": [
"error",
{
"requireCustomId": true,
"requireCustomIdMatchingPattern": "custom_id"
}
]
}We potentially could then take it a step further and dynamically default requireCustomId to true when requireCustomIdMatchingPattern?
There was a problem hiding this comment.
With that said, I am not super clear on the examples you have of:
boundTextAllowedPattern:
'^(?<lib>.[^\\.]*)\\.(?<component>.[^\\.]*)\\.(?<tag>.[^\\.]*)\\.(?<attribute>.[^\\.]*)?\\.(?<name>.[^\\.]*).$',
Are all of the sections potentially applicable to just the custom ID in practice? (I am not somebody who has ever leveraged the i18n feature personally). Maybe you could please give some real world examples?
There was a problem hiding this comment.
With that said, I am not super clear on the examples you have of:
boundTextAllowedPattern: '^(?<lib>.[^\\.]*)\\.(?<component>.[^\\.]*)\\.(?<tag>.[^\\.]*)\\.(?<attribute>.[^\\.]*)?\\.(?<name>.[^\\.]*).$',Are all of the sections potentially applicable to just the custom ID in practice? (I am not somebody who has ever leveraged the i18n feature personally). Maybe you could please give some real world examples?
I thought that it could be interesting to give some example to the developers to avoid duplicated custom ids and also to provide some more information about where the custom id is used.
Here you have an example of a custom id matching the regular expression: "cards-management.card-details-form.input.placeholder.remaining-credit"
There was a problem hiding this comment.
I don't think this is very clear that it relates to custom ID.
I think something like this would be much clearer:
{ "@angular-eslint/require-localizae-metadata": [ "error", { "requireCustomId": true, "requireCustomIdMatchingPattern": "custom_id" } ] }We potentially could then take it a step further and dynamically default
requireCustomIdto true whenrequireCustomIdMatchingPattern?
I have changed the name of the property to the one you have suggested.
How would you like to dynamically default requireCustomId to true?
There was a problem hiding this comment.
You changed the messageId but not the option which is the more prominent user-facing thing that I was concerned about.
For a dynamic default, you could just not specify it on the schema and apply it within the rule implementation at runtime, right?
| @@ -168,15 +205,260 @@ export const invalid: readonly InvalidTestCase<MessageIds, Options>[] = [ | |||
| ~~~~~~~~~~~~~~ | |||
| `, | |||
| messages: [ | |||
| { | |||
| char: '~', | |||
There was a problem hiding this comment.
Why did this existing test change?
There was a problem hiding this comment.
I found it was needed for the tests to pass green.
There was a problem hiding this comment.
I'd need to look at the test and your changes in detail, but I probably suggests a breaking change was introduced with this PR which we would want to avoid. Maybe something needs to be off by default? If you could at least please get to the bottom of why this change was needed we could then discuss if the change of behaviour is minor/objectively desired enough for it to go in as is, otherwise it might need to be worked around and changed in a major
|
@tcorral Would you like some help in finalizing this one? |
|
@JamesHenry yes, please. |
Added two more options to report errors if:
Change in details:
validatefunction that receives the input string and a type to validate optionally it can receive a pattern to validate custom id.validatefunction is a regular expression with named group, I have followed how Angular manages metadata when using$localizeNew config after changes:
requireCustomId-boolean- default isfalserequireDefaultValue-boolean- default isfalseboundTextAllowedPattern-string- No default value. It expects to get a string that can be converted to regular expression to test custom id. I have used the same name that already exists in@angular-eslint/template-parser.