Improved SyntaxFactory for documentation comments (Issue #218) #6790
Conversation
…xmaple elements in xml documentation comments. Add some unit tests.
…tion of xml documentation comment syntax).
davkean
added
the
Community
|
tag @dotnet/roslyn-compiler @mattwar |
|
test eta please |
|
Can I help testing eta or is it internal? |
|
@robinsedlaczek Sorry, that's a command to our internal bot :) @davkean is asking for the automated internal test run to kick in. |
|
Got it! Was my hope. :) Many thx! |
|
Sorry, just realized that could be misinterpreted. I'll prefix @dotnet-bot with it from now on. :) |
|
Cool, you're welcome. May I ask, what eta means? Just for my understanding. |
|
"Editor Test App". It's a cheap way to run our editor tests without requiring Visual Studio. |
|
@davkean Thx! So hopefully you accept the PR. |
|
Yep, the @dotnet/roslyn-compiler team will look over it when they have a chance. |
|
Can someone from the @dotnet/roslyn-compiler team look at this? |
|
Also tagging @AnthonyDGreen and @mattwar |
|
Looks good to me; This does make a lot of additions to the public API though, so I think it'll require API review. |
There was a problem hiding this comment.
You should be using elastic trivia when creating nodes, so the formatter will correctly format them later. This is usually automatically specified when calling overloads that do not specify trivia. It looks like the SyntaxFactory does not have an overload that adds the elastic trivia. You could add that too. Look at the implementation of the Literal factory method for an example.
|
There is no corresponding change to the VB syntax factory. It has XML doc comment too. However, it also has XML nodes. So its likely the naming used for some of these factory methods will become ambiguous if not somehow changed to imply documentation comments and not any XML. |
|
@mattwar Do you have some recommendations? |
|
On names that is. |
|
Also tagging @DustinCampbell |
There was a problem hiding this comment.
All of the string literals used here should use the constants from DocumentationCommentXmlNames.
There was a problem hiding this comment.
Ok, thanks for the hint!
…eating xml documentation comments. Use elastic trivias when creating syntax token (in C# and VB SyntaxFactory). Use xml names from DocumentationCommentXmlNames class.
|
Hi together! A happy new year to all of you. So here is my solution for the new line problem stated by @AnthonyDGreen and @mattwar. |
There was a problem hiding this comment.
@mattwar @AnthonyDGreen Now, this code concerns me a bit. The XmlMultiLineElement factory method is the first method that generates multiline code (as far as I can see). And I need to use this method within the higher level factory methods like XmlSummaryElement. So I have to give some kind of new line text as parameter value to the XmlNewLine factory method. So I decided to use "\r\n". Alternatively, we have to pass a text parameter for the new line text to the XmlSummaryElement factory method as well. What would you suggest to handle this situation?
There was a problem hiding this comment.
I'm not sure why this API needs to inject the new lines.. Shouldn't that be supplied as part of the content?
There was a problem hiding this comment.
Yes it should and now it is.
|
Following the log it seems that something is wrong with the build machines. Build processes for other PRs seems to run into same or similar problems in the last minutes. Compilation and unit tests were successful here locally. Can I help somehow? |
|
there was some problem with build machines. As i understand it should be fixed now. |
|
@VSadov Ok, thanks for the info! How are builds triggered again in such cases? |
|
Problems with the build machine. Mac run out of disk space. On Linux csc exited with code 134. |
|
@dotnet-bot test this please |
|
Any news regarding the health of the build machines? |
|
@dotnet-bot test prtest/mac/dbg/unit32 please |
|
@robinsedlaczek It looks like we are actually having trouble with the linux and mac tests A bug has been filed here #7854 |
|
You changed the file mode of build/scripts/tests.sh from 100755 to 100644 which is making it not executable. This is why the tests are failing. Please fix this and it'll kick off the tests again. |
|
@dotnet-bot test prtest/mac/dbg/unit32 please |
|
Finally we got it. @TyOverby Many thanks for your help! Hopefully, the PR can and will be accepted now. :) |
|
Can more person to sign off from @dotnet/roslyn-ide @dotnet/roslyn-compiler, please? |
|
👍 from me |
|
Thanks for the contribution! |
Improved SyntaxFactory for documentation comments (Issue #218)
|
Okay, now where's the party? |
There was a problem hiding this comment.
What's the plan for resolving this TODO? @jaredpar? I think the ban against LINQ is using IEnumerable operators. System.Xml.Linq is really not the concern by that rule.
There was a problem hiding this comment.
Yes, the ban is about not using LINQ queries in hot paths like parser and binder, because of the allocations. But they get used a lot in IDE layers.
|
🎉 |
|
Many thanks for all the support and for accepting the PR! Now, here is the party! ✨ 🎆 🎉 ✨ 🎉 🎆 ✨ 🎉 |
Avoid reporting CA1823 for inline arrays
11 participants
I've implemented the SyntaxFactory methods suggested in #218. Additionally, I added factory methods for the example and permission xml elements for documentation comments. And there are some tests for the new factory methods.