Skip to content

feat: add XML validation and quickfix for hardcoded UI text#491

Open
fausto-m wants to merge 6 commits into
SAP:masterfrom
fausto-m:master
Open

feat: add XML validation and quickfix for hardcoded UI text#491
fausto-m wants to merge 6 commits into
SAP:masterfrom
fausto-m:master

Conversation

@fausto-m

@fausto-m fausto-m commented Sep 6, 2022

Copy link
Copy Markdown

This is a validator that warns the user of hardcoded UI texts in XML views/fragments that could be externalized to a resource bundle. Includes a quick fix for the warning, where externalized strings from the project's i18n.properties file (if existent) may be suggested as replacements.

This is a validator that warns the user of hardcoded UI texts that could be externalized to a
resource bundle. Includes a quick fix for the warning, where externalized strings from the project's
i18n.properties file (if existent) may be suggested as replacements.
@lgtm-com

lgtm-com Bot commented Sep 6, 2022

Copy link
Copy Markdown

This pull request introduces 1 alert when merging 5c5968d into 9d0c89c - view on LGTM.com

new alerts:

  • 1 for Polynomial regular expression used on uncontrolled data

@bd82

bd82 commented Sep 29, 2022

Copy link
Copy Markdown
Member

@fausto-m the build matrix fails, can you please fix so I can start the review process.

@petermuessig WDYT of this feature?

@lgtm-com

lgtm-com Bot commented Sep 29, 2022

Copy link
Copy Markdown

This pull request introduces 1 alert when merging 930cd5f into 9d0c89c - view on LGTM.com

new alerts:

  • 1 for Polynomial regular expression used on uncontrolled data

The quick fix was using the value of a key-value pair in the i18n properties file instead of the key
@lgtm-com

lgtm-com Bot commented Sep 29, 2022

Copy link
Copy Markdown

This pull request introduces 1 alert when merging 70539f8 into 9d0c89c - view on LGTM.com

new alerts:

  • 1 for Polynomial regular expression used on uncontrolled data

lodash find was imported but never used
@lgtm-com

lgtm-com Bot commented Sep 29, 2022

Copy link
Copy Markdown

This pull request introduces 1 alert when merging 44709f1 into 9d0c89c - view on LGTM.com

new alerts:

  • 1 for Polynomial regular expression used on uncontrolled data

Also updates properties-file package dependency to a version that supports node18
@lgtm-com

lgtm-com Bot commented Sep 30, 2022

Copy link
Copy Markdown

This pull request introduces 1 alert when merging a069a2b into 9d0c89c - view on LGTM.com

new alerts:

  • 1 for Polynomial regular expression used on uncontrolled data

…olve conflict

Previous attribute conflicted with use-of-hardcoded-string-i18n validation as a GUI text property
was used in the test. It was replaced by another deprecated property of m.Page class.
@lgtm-com

lgtm-com Bot commented Oct 3, 2022

Copy link
Copy Markdown

This pull request introduces 1 alert when merging f1335dc into 90807cd - view on LGTM.com

new alerts:

  • 1 for Polynomial regular expression used on uncontrolled data

@fausto-m

fausto-m commented Oct 3, 2022

Copy link
Copy Markdown
Author

Hi @bd82, the build matrix issues have been solved now. Node16 failed due to something unrelated to my changes (timeout error from other test). Is there a way to rerun the test without additional commits? I've tried from CircleCI, but the option is disabled for me.

@uxkjaer

uxkjaer commented Oct 26, 2022

Copy link
Copy Markdown
Member

Hi @fausto-m,

I checked out this code and the validation is working fine. However I couldn't get the quick fix to work. Can you explain in a bit more detail how that works? Maybe we should add that to the readme.

I wonder if we should have a setting as well to enable or disable this validation, some projects are just in one language and it might seem as a nuisance for them to get these squiggly lines

@fausto-m

fausto-m commented Nov 16, 2022

Copy link
Copy Markdown
Author

Hi @uxkjaer,

Thanks for your feedback. You're right, opt-in is probably better for this feature. I'll work on that.

Regarding the quick fix part, you're supposed to get them if the (UI) text in a control property matches a value present in the project's resource bundle file. There's a script that locates the nearest i18n.properties file (nearest in relation to the XML view/fragment being displayed) and uses it for fetching suggestions. If needed, I can add more info to the readme file. Here's what it looks like:

Quick_Fix_for_i18n_l

@voicis

voicis commented Nov 29, 2022

Copy link
Copy Markdown
Contributor

Hi, I have a few more points to add regarding this feature.

Similar feature is implemented for annotation files in SAP Fiori Tools - XML Annotation Language Server and I think it would make sense to keep the behaviour as similar as possible across the extensions, since users could be using both in the same project.

  1. Properly resolving i18n model to its resource file - manifest.json contains information about the model names and where the resources are located at. Users can change the name of the i18n model or the path to the resource bundle and in that case current implementation may generate a code that actually doesn't work in runtime. Here is a sample fragment of manifest.json with the definition. This feat: introduce context package #523 PR might help with getting such information from the context.
"sap.ui5": {
        "models": {
            "i18n": {
                "type": "sap.ui.model.resource.ResourceModel",
                "settings": {
                    "bundleName": "sap.fe.cap.travel.i18n.i18n"
                }
            }
      }
}
  1. Current use case seems rather narrow, because it requires the translated text to exist which probably isn't the case most of the time. I would expect that a more common scenario would be one where you generate a new text (in i18n.properties file) for values that are not yet translated as well and save the user time for having to define one.
  2. I'm concerned that maintaining ui5-user-facing-attributes.ts could prove troublesome, because we need to track the changes done to control libraries. It would be good if we could get information which properties should be translated from the ui5 metadata, but it looks like currently it is missing.

@cla-assistant

cla-assistant Bot commented Oct 18, 2023

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

2 similar comments
@cla-assistant

cla-assistant Bot commented Oct 18, 2023

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@cla-assistant

cla-assistant Bot commented Oct 18, 2023

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants