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

Implemet conversion of XML namespace imports #852

Merged
merged 2 commits into from
Mar 18, 2022

Conversation

Noah1989
Copy link
Contributor

Link to issue(s) this covers

This PR implements the approach suggested by me in that issue. I decided to keep it at one helper class per file (if needed) to keep the scope where imports apply the same as in VB.

Problem

XML import statements are not converted.

Solution

  • This solution tries to mimic the behavior of VB XML namespace imports.
  • If any XML namespace imports are present, a helper class is generated per file, which contains the namespace definitions. This is in line with how they apply in VB. This also keeps them at the top of the file and allows for changing them easily after conversion.
  • The document name is included in the generated helper class name in order to avoid duplicate names while staying deterministic. A using directive is generated to provide a shorter (common) alias.
  • When converting XML names with prefixes, the namespace definitions from the helper class are used.
  • The helper class is also used to manage xmlns attributes. Whenever a XContainer is created, they are added, and any dupicate attributes from the child elements are removed. This is in line with what the helper code generated by VB does.
  • At least one test covering the code changed

@GrahamTheCoder GrahamTheCoder merged commit 4b4bff2 into icsharpcode:master Mar 18, 2022
@GrahamTheCoder
Copy link
Member

I'm recovering from quite a bad bout of covid at the moment so don't have the energy to fully understand and give this the in depth review I was hoping to. But from just glancing over it, it's very clearly a big improvement, and looks like good quality code. Thanks a lot for your efforts!

}

public override async Task<CSharpSyntaxNode> VisitXmlString(VBSyntax.XmlStringSyntax node) =>
CommonConversions.Literal(node.TextTokens.Aggregate("", (a, b) => a + LiteralConversions.EscapeVerbatimQuotes(b.Text)));
Copy link
Member

Choose a reason for hiding this comment

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

Not sure of the likely inputs here myself, but if there could be quite a few strings and especially if they could be big, consider StringBuilder

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. I guess String.Join could also work, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, that'd probably be slightly more readable too actually 👍

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