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: Improve extraction of inline XML #547

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

matz3
Copy link
Member

@matz3 matz3 commented Feb 17, 2025

The previous implementation extracted XML content from every options
property, not just the actual XML definition/content.
This lead to unnecessary empty inline JS files.

Follow-up of #519

@matz3 matz3 requested a review from a team February 17, 2025 20:00
d3xter666
d3xter666 previously approved these changes Feb 19, 2025
The previous implementation extracted XML content from every options
property, not just the actual XML definition/content.
This lead to unnecessary empty inline JS files.

Follow-up of #519
{
column: 17,
line: 33,
message: 'Call to deprecated function \'sapUiXmlView\'',
Copy link
Contributor

Choose a reason for hiding this comment

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

The deprecation is correct, but to me this seems a bit missleading. There's no such thing as sapUiXmlView, but its assignment. While in the example, the solution is obvious, in real code this might not be the case and might lead to ambiguous situations

Copy link
Member Author

Choose a reason for hiding this comment

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

We might try to enhance this in general by adding the original API name to the message. This is sometimes working, but I think in this case it does not, as the additionalMessage is empty.

@matz3 matz3 force-pushed the improve-inline-xml-detection branch from 88a7fb3 to df6c3ef Compare February 20, 2025 09:00
@matz3 matz3 requested a review from codeworrior February 20, 2025 09:01
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