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.

Sign up for GitHub

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

[generator] Handle multiple @return javadoc values #836

Merged
merged 2 commits into from
May 12, 2021
Merged

[generator] Handle multiple @return javadoc values #836

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5fbd7b2
[generator] Handle multiple @return javadoc values
pjcollins May 11, 2021
b21e665
Apply feedback
pjcollins May 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions ...avaSource/Java.Interop.Tools.JavaSource/SourceJavadocToXmldocGrammar.BlockTagsBnfTerms.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,10 +114,23 @@ internal void CreateRules (SourceJavadocToXmldocGrammar grammar)
if (!grammar.ShouldImport (ImportJavadoc.ReturnTag)) {
return;
}
var r = new XElement ("returns",
// When encountering multiple @return keys in a line, append subsequent @return key content to the original <returns> element.
var jdi = FinishParse (context, parseNode);
if (jdi.Returns.Count == 0) {
var r = new XElement ("returns",
AstNodeToXmlContent (parseNode.ChildNodes [1]));
FinishParse (context, parseNode).Returns.Add (r);
parseNode.AstNode = r;
FinishParse (context, parseNode).Returns.Add (r);
parseNode.AstNode = r;

} else {
var r = jdi.Returns.First () as XElement;
if (r != null) {
Copy link
Member

Choose a reason for hiding this comment

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

This can't be null; .First() requires that an element be returned, and unless we have a bug and we do .Returns.Add(null), there should only be non-null elements within jdi.Returns.

If we want to allow null, use .FirstOrDefault().

Actually, on review this entire block doesn't quite make sense to me: r itself shouldn't change from r.Add(), and thus this should be a no-op (unless jdi.Returns somehow has > 1 element in it? But how could that happen?)

FinishParse (context, parseNode).Returns.Clear ();
FinishParse (context, parseNode).Returns.Add (r);

Neither of the above statements should be needed.

Copy link
Member Author

@pjcollins pjcollins May 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The null check was intended to handle the possibility of jdi.Returns.First () not being an XElement, though I think we only ever add XElements to this ICollection<XNode> in the section above and so the check probably shouldn't be needed.

The FinishParse (context, parseNode).Returns collection will have more than zero elements in it when the <javadoc> entry that is being parsed has multiple @return values in it, as shown in the test addition below. The Returns.Clear and Returns.Add additions do look to be redundant here though, I will clean that up.

r.Add (" ", AstNodeToXmlContent (parseNode.ChildNodes [1]));
FinishParse (context, parseNode).Returns.Clear ();
FinishParse (context, parseNode).Returns.Add (r);
parseNode.AstNode = r;
}
}
};

SeeDeclaration.Rule = "@see" + BlockValues;
Expand Down
39 changes: 24 additions & 15 deletions tests/Java.Interop.Tools.JavaSource-Tests/SourceJavadocToXmldocParserTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,21 +17,19 @@ namespace Java.Interop.Tools.JavaSource.Tests
[TestFixture]
public class SourceJavadocToXmldocParserTests : SourceJavadocToXmldocGrammarFixture {

[Test]
public void TryParse ()
[Test, TestCaseSource (nameof (TryParse_Success))]
public void TryParse (ParseResult parseResult)
{
foreach (var values in TryParse_Success) {
ParseTree parseTree;
var p = new SourceJavadocToXmldocParser (XmldocStyle.Full);
var n = p.TryParse (values.Javadoc, null, out parseTree);
Assert.IsFalse (parseTree.HasErrors (), DumpMessages (parseTree, p));
Assert.AreEqual (values.FullXml, GetMemberXml (n), $"while parsing input: ```{values.Javadoc}```");
ParseTree parseTree;
var p = new SourceJavadocToXmldocParser (XmldocStyle.Full);
var n = p.TryParse (parseResult.Javadoc, null, out parseTree);
Assert.IsFalse (parseTree.HasErrors (), DumpMessages (parseTree, p));
Assert.AreEqual (parseResult.FullXml, GetMemberXml (n), $"while parsing input: ```{parseResult.Javadoc}```");

p = new SourceJavadocToXmldocParser (XmldocStyle.IntelliSense);
n = p.TryParse (values.Javadoc, null, out parseTree);
Assert.IsFalse (parseTree.HasErrors (), DumpMessages (parseTree, p));
Assert.AreEqual (values.IntelliSenseXml, GetMemberXml (n), $"while parsing input: ```{values.Javadoc}```");
}
p = new SourceJavadocToXmldocParser (XmldocStyle.IntelliSense);
n = p.TryParse (parseResult.Javadoc, null, out parseTree);
Assert.IsFalse (parseTree.HasErrors (), DumpMessages (parseTree, p));
Assert.AreEqual (parseResult.IntelliSenseXml, GetMemberXml (n), $"while parsing input: ```{parseResult.Javadoc}```");
}

static string GetMemberXml (IEnumerable<XNode> members)
Expand All @@ -40,7 +38,7 @@ static string GetMemberXml (IEnumerable<XNode> members)
return e.ToString ();
}

static readonly ParseResult[] TryParse_Success = new ParseResult[]{
public static readonly ParseResult[] TryParse_Success = new ParseResult[]{
new ParseResult {
Javadoc = "Summary.\n\nP2.\n\n<p>Hello!</p>",
FullXml = @"<member>
Expand Down Expand Up @@ -78,6 +76,17 @@ static string GetMemberXml (IEnumerable<XNode> members)
<returns>
<c>true</c> if something
or other; otherwise <c>false</c>.</returns>
</member>",
},
new ParseResult {
Javadoc = "@return {@code true} if something else @return {@code false}.",
Copy link
Member

Choose a reason for hiding this comment

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

I'm half surprised that @tag parsing isn't requiring newlines in there, and that we don't need \n@return ……

FullXml = @"<member>
<returns>
<c>true</c> if something else <c>false</c>.</returns>
</member>",
IntelliSenseXml = @"<member>
<returns>
<c>true</c> if something else <c>false</c>.</returns>
</member>",
},
new ParseResult {
Expand Down Expand Up @@ -166,7 +175,7 @@ more description here.</para>
},
};

class ParseResult {
public class ParseResult {
public string Javadoc;
public string FullXml;
public string IntelliSenseXml;
Expand Down