-
Notifications
You must be signed in to change notification settings - Fork 59
Feat: add sap-description-column-label ESLint rule #4542
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
marcovieth
wants to merge
20
commits into
main
Choose a base branch
from
feat/rule-descriptionColumnLabel
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 8 commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
e1e0145
feat: add sap-description-column-label ESLint rule
marcovieth f719c88
Merge remote-tracking branch 'origin/main' into feat/rule-description…
marcovieth 9107455
add JSDoc to some functions
marcovieth 5418ac5
feat: enhance OData V2 support by injecting inline sap:text and sap:l…
marcovieth 63b54e6
added tests for sap:text and sap:label in V2 property parsing
marcovieth 1503f4f
update changeset
marcovieth ffe2818
feat: add V2 rule tests for inline sap:text and sap:label
marcovieth cc3cfa5
docs: add V2 examples, use writing style "xxx annotation"
marcovieth 025449a
fix: update import path for AnnotationFile type in service tests
marcovieth 90a3db5
docs: enhance rule description
marcovieth 92eebfa
Merge branch 'main' into feat/rule-descriptionColumnLabel
marcovieth 583d5d4
refactor: move utility functions to common-text-helpers module
marcovieth 69b05b0
Merge branch 'main' into feat/rule-descriptionColumnLabel
marcovieth beb471f
fix: handle undefined label annotations and improve trivial label che…
marcovieth 62b4955
Merge branch 'main' into feat/rule-descriptionColumnLabel
marcovieth 49af900
docs: removed separate Examples section and improved writing style in…
marcovieth d0f7c68
refactor: simplify annotation key parsing and improve utility functions
marcovieth 0c4984c
Merge branch 'main' into feat/rule-descriptionColumnLabel
marcovieth 3fa3d9d
fix: replace annotation keys with fully-qualified term names in tests
marcovieth 5d8bc7a
refactor: show issues on the Common.Label annotation and not on the C…
marcovieth File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| '@sap-ux/xml-odata-annotation-converter': minor | ||
| '@sap-ux/odata-annotation-core-types': minor | ||
| '@sap-ux/eslint-plugin-fiori-tools': minor | ||
| --- | ||
|
|
||
| [rule] Add rule to check that a Common.Text description property has a meaningful Common.Label | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
135 changes: 135 additions & 0 deletions
135
packages/eslint-plugin-fiori-tools/docs/rules/sap-description-column-label.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,135 @@ | ||||||
| # Require Meaningful Labels for Description (Text) Properties (`sap-description-column-label`) | ||||||
|
|
||||||
| Ensures that the property referenced as the description (text) value via the `Common.Text` annotation has a meaningful `Common.Label` annotation — not a generic value such as `"Name"` or `"Description"`, and not the same label as the ID property it describes. | ||||||
|
marcovieth marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| ## Rule Details | ||||||
|
|
||||||
| When a field uses the `Common.Text` annotation to associate a human-readable description property with a technical key property, the description property should carry a `Common.Label` annotation that clearly identifies its content. A generic label like `"Name"` or `"Description"` provides no useful context to the user in the UI (for example, as a column header in a table), and a label that is identical to the ID property's label creates ambiguity between the two columns. | ||||||
|
marcovieth marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| The rule checks every `Common.Text` annotation on a property that belongs to an entity type used in the application (i.e. linked to at least one page via the manifest). For each such annotation the rule: | ||||||
|
marcovieth marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| 1. Resolves the path to the referenced text property (including navigation segments such as `category/name`). | ||||||
|
marcovieth marked this conversation as resolved.
Outdated
|
||||||
| 2. Reads the `Common.Label` annotation on that property. | ||||||
| 3. Flags the annotation if the label is `"Name"` or `"Description"` (case-insensitive) — **trivialLabel**. | ||||||
|
marcovieth marked this conversation as resolved.
Outdated
|
||||||
| 4. Flags the annotation if the label is identical (case-insensitive) to the `Common.Label` annotation of the ID property that carries the `Common.Text` annotation — **duplicateLabel**. | ||||||
|
marcovieth marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| If the text property has no `Common.Label` annotation, the rule does not flag it. | ||||||
|
marcovieth marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| > **OData V2**: The rule also applies to OData V2 services. In V2 metadata, the `sap:text` attribute on a `Property` element is treated as an implicit `Common.Text` annotation, and the `sap:label` attribute is treated as an implicit `Common.Label` annotation. The rule reports on the metadata file directly when these inline attributes produce a trivial or duplicate label. | ||||||
|
marcovieth marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| ### Why Was This Rule Introduced? | ||||||
|
|
||||||
| When a property is displayed using a description column (e.g. `TextArrangement/TextOnly`), the column header comes from the `Common.Label` annotation of the text property. A label of `"Name"` or `"Description"` is semantically meaningless in that context — it does not tell the user _which_ name or description is shown. Similarly, if the description column carries the exact same label as the ID column, the user cannot distinguish the two columns. | ||||||
|
marcovieth marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| ### Warning Messages | ||||||
|
|
||||||
| **trivialLabel** | ||||||
|
marcovieth marked this conversation as resolved.
Outdated
|
||||||
| ``` | ||||||
| The text property "{{textPropertyTarget}}" has a generic label "{{textPropertyLabel}}". Use a more descriptive label that distinguishes it from other properties. | ||||||
|
marcovieth marked this conversation as resolved.
Outdated
|
||||||
| ``` | ||||||
|
|
||||||
| **duplicateLabel** | ||||||
|
marcovieth marked this conversation as resolved.
Outdated
|
||||||
| ``` | ||||||
| The text property "{{textPropertyTarget}}" has the same label "{{textPropertyLabel}}" as the ID property "{{idPropertyTarget}}". The description column label should be different from the ID label. | ||||||
|
marcovieth marked this conversation as resolved.
Outdated
|
||||||
| ``` | ||||||
|
|
||||||
| ## Examples | ||||||
|
|
||||||
| ### Incorrect Annotations | ||||||
|
|
||||||
| **`trivialLabel` — the `Common.Label` annotation on the text property is too generic:** | ||||||
|
marcovieth marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| ```xml | ||||||
| <!-- ProductBaseUnit uses UnitOfMeasure_Text as its description column via Common.Text annotation --> | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done with commit 49af900 |
||||||
| <Annotations Target="MyService.MyEntity/ProductBaseUnit"> | ||||||
| <Annotation Term="Common.Text" Path="to_BaseUnit/UnitOfMeasure_Text"/> | ||||||
| </Annotations> | ||||||
|
|
||||||
| <!-- The Common.Label annotation on the description property is "Name" — too generic --> | ||||||
|
marcovieth marked this conversation as resolved.
Outdated
|
||||||
| <Annotations Target="MyService.BaseUnitType/UnitOfMeasure_Text"> | ||||||
| <Annotation Term="Common.Label" String="Name"/> | ||||||
| </Annotations> | ||||||
| ``` | ||||||
|
|
||||||
| **`duplicateLabel` — the `Common.Label` annotation on the text property is identical to the ID property label:** | ||||||
|
marcovieth marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| ```xml | ||||||
| <!-- Supplier uses CompanyName as its description column via Common.Text annotation, both properties have label "Supplier" --> | ||||||
|
marcovieth marked this conversation as resolved.
Outdated
|
||||||
| <Annotations Target="MyService.MyEntity/Supplier"> | ||||||
| <Annotation Term="Common.Text" Path="to_Supplier/CompanyName"/> | ||||||
| <Annotation Term="Common.Label" String="Supplier"/> | ||||||
| </Annotations> | ||||||
|
|
||||||
| <!-- Common.Label annotation identical to the ID property above — ambiguous column headers --> | ||||||
|
marcovieth marked this conversation as resolved.
Outdated
|
||||||
| <Annotations Target="MyService.SupplierType/CompanyName"> | ||||||
| <Annotation Term="Common.Label" String="Supplier"/> | ||||||
| </Annotations> | ||||||
| ``` | ||||||
|
|
||||||
| **OData V2 `trivialLabel` — inline `sap:label` on the text property is too generic (reported on the metadata file):** | ||||||
|
marcovieth marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| ```xml | ||||||
| <!-- sap:text attribute acts as an implicit Common.Text annotation --> | ||||||
| <Property Name="Product" Type="Edm.String" sap:text="ProductName" sap:label="Product ID" /> | ||||||
| <!-- sap:label="Name" acts as an implicit Common.Label annotation — too generic --> | ||||||
| <Property Name="ProductName" Type="Edm.String" sap:label="Name" /> | ||||||
| ``` | ||||||
|
|
||||||
| **OData V2 `duplicateLabel` — inline `sap:label` values on ID and text property are identical:** | ||||||
|
marcovieth marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| ```xml | ||||||
| <!-- Both properties carry sap:label="Quantity Unit" via inline sap:label attribute --> | ||||||
|
marcovieth marked this conversation as resolved.
Outdated
|
||||||
| <Property Name="QuantityUnit" Type="Edm.String" sap:text="QuantityUnitT" sap:label="Quantity Unit" /> | ||||||
| <Property Name="QuantityUnitT" Type="Edm.String" sap:label="Quantity Unit" /> | ||||||
| ``` | ||||||
|
|
||||||
| ### Correct Annotations | ||||||
|
|
||||||
| **Meaningful, distinct `Common.Label` annotation on the text property:** | ||||||
|
marcovieth marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| ```xml | ||||||
| <Annotations Target="MyService.MyEntity/ProductBaseUnit"> | ||||||
| <Annotation Term="Common.Text" Path="to_BaseUnit/UnitOfMeasure_Text"/> | ||||||
| </Annotations> | ||||||
|
|
||||||
| <!-- Common.Label annotation with a descriptive value that distinguishes the column from the ID column --> | ||||||
| <Annotations Target="MyService.BaseUnitType/UnitOfMeasure_Text"> | ||||||
| <Annotation Term="Common.Label" String="Unit of Measure"/> | ||||||
| </Annotations> | ||||||
| ``` | ||||||
|
|
||||||
| **Different `Common.Label` annotations on ID and text properties:** | ||||||
|
marcovieth marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| ```xml | ||||||
| <Annotations Target="MyService.MyEntity/Supplier"> | ||||||
| <Annotation Term="Common.Text" Path="to_Supplier/CompanyName"/> | ||||||
| <Annotation Term="Common.Label" String="Supplier ID"/> | ||||||
| </Annotations> | ||||||
|
|
||||||
| <Annotations Target="MyService.SupplierType/CompanyName"> | ||||||
| <Annotation Term="Common.Label" String="Company Name"/> | ||||||
| </Annotations> | ||||||
| ``` | ||||||
|
|
||||||
| **OData V2 — distinct `sap:label` values on ID and text property:** | ||||||
|
marcovieth marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| ```xml | ||||||
| <Property Name="QuantityUnit" Type="Edm.String" sap:text="QuantityUnitT" sap:label="Quantity Unit" /> | ||||||
| <!-- Distinct sap:label — unambiguous column headers --> | ||||||
|
marcovieth marked this conversation as resolved.
Outdated
|
||||||
| <Property Name="QuantityUnitT" Type="Edm.String" sap:label="Unit of Measure Text" /> | ||||||
| ``` | ||||||
|
|
||||||
| ## Bug Report | ||||||
|
|
||||||
| If you encounter an issue with this rule, please open a [GitHub issue](https://github.com/SAP/open-ux-tools/issues). | ||||||
|
|
||||||
| ## When Not to Disable This Rule | ||||||
|
|
||||||
| This rule should not be disabled unless you have a confirmed backend constraint that prevents renaming the label. In most cases the fix is straightforward: update the `Common.Label` annotation (or the `sap:label` attribute in OData V2 metadata) on the text property to something meaningful and distinct from the ID property label. | ||||||
|
marcovieth marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| ## Further Reading | ||||||
|
|
||||||
| - [UI5 Further Features of the Field - OData V4](https://ui5.sap.com/#/topic/f49a0f7eaafe444daf4cd62d48120ad0) | ||||||
| - [UI5 Displaying Text and ID for Value Help Input Fields - OData V2](https://ui5.sap.com/#/topic/080886d8d4af4ac6a68a476beab17da3) | ||||||
| - [OData Common Vocabulary - Text](https://github.com/SAP/odata-vocabularies/blob/main/vocabularies/Common.md#Text) | ||||||
| - [OData Common Vocabulary - Label](https://github.com/SAP/odata-vocabularies/blob/main/vocabularies/Common.md#Label) | ||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| export const UI_LINE_ITEM = 'com.sap.vocabularies.UI.v1.LineItem'; | ||
| export const COMMON_TEXT = 'com.sap.vocabularies.Common.v1.Text'; | ||
| export const COMMON_LABEL = 'com.sap.vocabularies.Common.v1.Label'; | ||
| export const UI_HIDDEN = 'com.sap.vocabularies.UI.v1.Hidden'; | ||
| export const UI_TEXT_ARRANGEMENT = 'com.sap.vocabularies.UI.v1.TextArrangement'; | ||
| export const UI_FIELD_GROUP = 'com.sap.vocabularies.UI.v1.FieldGroup'; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.