Skip to content
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

fix: relax diagnostic for special property #708

Merged
merged 4 commits into from
Apr 23, 2024

Conversation

marufrasully
Copy link
Contributor

Issue: #707

Description

Some property e.g tooltip can be used with both aggregation binding info or property binding info. Aggregation binding in this case requires mandatory property e.g path where property binding requires no mandatory property. Application user can decide to use either aggregation binding info or property binding info. This PR relax diagnostics check. If there is any element used, diagnostic for mandatory property is not checked.

Copy link

changeset-bot bot commented Apr 22, 2024

🦋 Changeset detected

Latest commit: f3144a7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@ui5-language-assistant/vscode-ui5-language-assistant-bas-ext Patch
vscode-ui5-language-assistant Patch
@ui5-language-assistant/binding Patch
@ui5-language-assistant/language-server Patch

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

Copy link

sonarcloud bot commented Apr 22, 2024

if (!usedRequiredEl && altTypes) {
// some property e.g `tooltip` can be used with both `aggregation binding info` or `property binding info`. Therefore `altTypes` is defined in design time.
// if `altTypes` is present, check if any element is used. This is a very broad check to avoid false diagnostic.
usedRequiredEl = element.elements[0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does elements array always contain at least one element?
Maybe better here just exit the validation function with empty result?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the exception is to check if there is at least one element present from either aggregation property binding or property binding info. If that element is not present, we still want to show diagnostic for mandatory property.

@marufrasully marufrasully merged commit 98c33e7 into master Apr 23, 2024
9 checks passed
@marufrasully marufrasully deleted the fix/issue/707/relax-diagnostic branch April 23, 2024 12:10
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.

3 participants