Skip to content

fix: support namespace in aggregation name #150

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

Merged
merged 3 commits into from
Jun 1, 2020
Merged

Conversation

tal-sapan
Copy link
Contributor

Aggregation tags must have the same namespace as their parent control tag.
This is now supported in code completion, validations and hover.

Additionally, don't filter out the current aggregation during code completion.

Aggregation tags must have the same namespace as their parent control tag.
This is now supported in code completion, validations and hover.

Additionally, don't filter out the current aggregation during code
completion.
@tal-sapan tal-sapan requested a review from bd82 May 27, 2020 14:08
* Split possibly qualified tag or attribute name to namespace and name
* @param qName
*/
export function splitQNameByNamespace(
Copy link
Member

@bd82 bd82 May 27, 2020

Choose a reason for hiding this comment

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

  • This possibly belongs in XML-Tools
  • QName is ambiguous, is this "x.y.z" (regular fqn) or XMLNs fqn "foo:bar"
    There is no way to tell from the docs.
  • I know its the xmlns name... so in that case it should use xmlns terms, e.g:
    • ns --> prefix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • This possibly belongs in XML-Tools

Actually I agree, but I don't think I'll have time to move it this sprint.

  • QName is ambiguous, is this "x.y.z" (regular fqn) or XMLNs fqn "foo:bar"
    There is no way to tell from the docs.

I'll improve the comment to make it clearer that it's related to xml qualified names.

  • I know its the xmlns name... so in that case it should use xmlns terms, e.g:

    • ns --> prefix

Ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • I know its the xmlns name... so in that case it should use xmlns terms, e.g:

    • ns --> prefix

On the xml element it's called ns, even though it's the prefix and not the resolved namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

Copy link
Member

Choose a reason for hiding this comment

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

On the xml element it's called ns, even though it's the prefix and not the resolved namespace.

Yes that is a mistake I've made, but fixing that would be a serious breaking change

ns: matchGroups.ns,
name:
matchGroups.name ??
/* istanbul ignore next */
Copy link
Member

Choose a reason for hiding this comment

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

why is this branch ignored?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it's unnecessary, I'll remove the ??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

@bd82
Copy link
Member

bd82 commented May 27, 2020

Aggregation tags must have the same namespace as their parent control tag.

Do the namespace prefixes need to be syntactically equivalent or could they also be resolved to the same namespace? e.g:

  • Two xmlns for sap.m
  • default + explicit ns for sap.m

@tal-sapan
Copy link
Contributor Author

Do the namespace prefixes need to be syntactically equivalent or could they also be resolved to the same namespace?

I tested it now and it seems that they can be equivalent instead of the same prefix (non-default and default or both non-default - it works).
The namespace itself must be exactly the same though.
I will fix the logic.

As a side note, I could not get content or any other aggregations under mvc:View to work with or without a namespace. It always treats them as a class instead of aggregation.

@tal-sapan
Copy link
Contributor Author

Do the namespace prefixes need to be syntactically equivalent or could they also be resolved to the same namespace?

Fixed

@tal-sapan tal-sapan requested a review from bd82 May 31, 2020 15:49
@bd82
Copy link
Member

bd82 commented Jun 1, 2020

As a side note, I could not get content or any other aggregations under mvc:View to work with or without a namespace. It always treats them as a class instead of aggregation.

"Work" in what context?

* Return the xml namespace defined for the xml element prefix (ns), or undefined if not found
* @param xmlElement
*/
export function resolveXMLNS(xmlElement: XMLElement): string | undefined;
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

These are all very useful utils, we will need to move them to XML-Tools at a later time though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed

@tal-sapan
Copy link
Contributor Author

As a side note, I could not get content or any other aggregations under mvc:View to work with or without a namespace. It always treats them as a class instead of aggregation.

"Work" in what context?

It doesn't work, as in, I get a runtime error that "content" was not found, and it seems that it searches for it by URL like it's a class.

@tal-sapan tal-sapan merged commit cff718b into master Jun 1, 2020
@tal-sapan tal-sapan deleted the aggregation_with_ns branch June 1, 2020 11:07
@bd82
Copy link
Member

bd82 commented Jun 1, 2020

If we have a runtime error we need to open a bug with reproduceable test case

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.

2 participants