Move getFloorplanLabel to fiori generator shared - #4927
Conversation
🦋 Changeset detectedLatest commit: 1d4b8f9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 34 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
SummaryThe following content is AI-generated and provides a summary of the pull request: DescriptionMoves the Key changes:
Type of change
How have you tested?
Checklist:
PR Bot InformationVersion:
|
![hyperspace-pr-bot[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR cleanly achieves its goal of consolidating getFloorplanLabel into @sap-ux/fiori-generator-shared and removing the circular dependency from repo-app-import-sub-generator. The refactoring is well-structured with appropriate deprecation notices, updated tests, and a valid changeset. Two issues were flagged: the interpolation.format callback in fiori-generator-shared doesn't match the i18next function signature (missing the format name guard), and the fallback behaviour for unknown templates with an OData version warrants a clarifying comment or intentional assertion.
| interpolation: { | ||
| format: function odataVersionFormatter(odataVersion: string) { | ||
| return odataVersion ? ` V${odataVersion}` : ''; | ||
| } |
There was a problem hiding this comment.
Bug: The interpolation.format function signature does not match the i18next API contract.
The i18next format callback receives three arguments: (value, format, lng). The current implementation names the first parameter odataVersion, which only works because the i18n keys use a named formatter ({{odataVersion, odataVersionFormatter}}). However, i18next passes the interpolated variable's value as the first argument — not the format name. The format name (odataVersionFormatter) is passed as the second argument and is being silently ignored. This means the function works by accident: it returns `V${value}` for any truthy first argument, regardless of whether the formatter was actually intended.
Additionally, the function only handles the odataVersionFormatter format, but any other interpolated value calling a named formatter will also be processed here indiscriminately. The format name should be checked to guard this:
interpolation: {
format: function (value: string, format?: string) {
if (format === 'odataVersionFormatter') {
return value ? ` V${value}` : '';
}
return value;
}
},| interpolation: { | |
| format: function odataVersionFormatter(odataVersion: string) { | |
| return odataVersion ? ` V${odataVersion}` : ''; | |
| } | |
| interpolation: { | |
| format: function (value: string, format?: string) { | |
| if (format === 'odataVersionFormatter') { | |
| return value ? ` V${value}` : ''; | |
| } | |
| return value; | |
| } | |
| }, |
Double-check suggestion before committing. Edit this comment for amendments.
Thank you for submitting your feedback: ✅ Helpful comment.
|
|
||
| test('should return templateType as fallback for unknown template', () => { | ||
| expect(getFloorplanLabel('unknown-template')).toBe('unknown-template'); | ||
| expect(getFloorplanLabel('unknown-template', '4')).toBe('unknown-template'); |
There was a problem hiding this comment.
Bug: getFloorplanLabel('unknown-template', '4') is expected to return 'unknown-template', but the version suffix will actually be appended.
When the i18n key floorplans.label.unknown-template is missing, the defaultValue: templateType option kicks in. However, this default value string does not contain the {{odataVersion, odataVersionFormatter}} interpolation token, so the version suffix is never appended — and the test passes for the wrong reason. The real question is whether an unknown template with an OData version should return 'unknown-template V4'. If intentionally no suffix is desired for unknown templates, the current behaviour is correct but the test should document this intent explicitly. If a suffix is desired, the default value template needs to include the token. Either way, the test assertion masks a potential design gap.
Thank you for submitting your feedback: ❌ This comment is not helpful.
There was a problem hiding this comment.
But since there's no defined label for an unknown template, returning unknown-template is ok? — appending
' V4' to an already-meaningless string doesn't add value.
| "ovp": "Overview Page{{odataVersion, odataVersionFormatter}}", | ||
| "feop": "Form Entry Object Page{{odataVersion, odataVersionFormatter}}" | ||
| }, | ||
| "description": { |
There was a problem hiding this comment.
I am not sure if we need to move floorplans.description to fiori generator shared as well? I couldnt find where this is used in the repo as well 🤷
|
2 participants
getFloorplanLabelfromfiori-app-sub-generator/src/utils/common.tsto@sap-ux/fiori-generator-shared/src/app-gen-info.tsas a shared utilityfloorplans.labeltranslations andodataVersionFormatterinterpolation tofiori-generator-sharedi18nfloorplans.labeltranslation block fromfiori-app-sub-generatorepo-app-import-sub-generatorto importgetFloorplanLabelfrom shared instead offiori-app-sub-generator, removing the dependency entirely as flagged in PR comment