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

Typelookup clean up and add objects for parameters and similar symbols #1062

Merged
merged 9 commits into from
Dec 14, 2017

Conversation

akshita31
Copy link
Contributor

Issue : dotnet/vscode-csharp#1057

The fix removes unnecessary tags in the structured documentation.Also the Parameter, Exception and TypeParameters to objects having a name and documentation field. The tests have been modified accordingly.

Omnisharp-Vscode side : dotnet/vscode-csharp#1918

@akshita31
Copy link
Contributor Author

@DustinCampbell @rchande Do we want to remove the "Summary" and other tags from omnisharp-roslyn completely? More specifically from the DocumentationConverter.convertDocumentation as well ?

@DustinCampbell
Copy link
Contributor

Do we want to remove the "Summary" and other tags from omnisharp-roslyn completely? More specifically from the DocumentationConverter.convertDocumentation as well ?

I don't think so. That's still constructing the text for the Documentation property, so we should leave that for back-compat with other other clients.

Copy link

@rchande rchande left a comment

Choose a reason for hiding this comment

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

Looking pretty good, couple of suggestions.

public string ReturnsText { get; }
public string RemarksText { get; }
public string ExampleText { get; }
public string ValueText { get; }
public string[ ] Exception { get; }
public DocumentedObject[ ] Exception { get; }
Copy link

Choose a reason for hiding this comment

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

Extra space

@@ -10,15 +10,15 @@ namespace OmniSharp.Models.TypeLookup
public class DocumentationComment
{
public string SummaryText { get; }
public string[] TypeParamElements { get; }
public string[] ParamElements { get; }
public DocumentedObject[] TypeParamElements { get; }
Copy link

Choose a reason for hiding this comment

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

This could just be Array<KeyValuePair<string, string>> or Array<Tuple<string, string>>

Copy link

Choose a reason for hiding this comment

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

Thoughts @DustinCampbell ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what Newtownsoft.Json supports here. If we go with a custom type, I might call it DocumentationItem.

Copy link
Contributor

Choose a reason for hiding this comment

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

@akshita31 : Note that Array is not a thing in C#. @rchande meant KeyValuePair<string, string>>[] and Tuple<string, string>[]. I would lean away from tuple because it would mean that the protocol would have weird names like Item1 and Item2 in it, which isn't what we want.

@@ -166,4 +159,25 @@ private static string GetCref(string cref)
return cref + " ";
}
}

class DocumentedObjectHelper
Copy link

Choose a reason for hiding this comment

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

DocumentedObjectBuilder?

public string Name { get; set; }
public StringBuilder Documentation { get; set; }

public DocumentedObjectHelper()
Copy link

Choose a reason for hiding this comment

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

extra spaces

Documentation = new StringBuilder();
}

public DocumentedObject ConvertToDocumentedObject()
Copy link

Choose a reason for hiding this comment

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

extra spaces

{
public class DocumentedObject
{
public string Name { get; set; }
Copy link

Choose a reason for hiding this comment

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

Can we get rid of the public setters? This type should be immutable.

Name = Name,
Documentation = Documentation.ToString()
};
return documentedObject;
Copy link

Choose a reason for hiding this comment

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

Newline after close brace of block when followed by non-brace.

@@ -140,7 +132,8 @@ public static DocumentationComment From(string xmlDocumentation, string lineEndi
return null;
}
}
return new DocumentationComment(summaryText.ToString(), typeParamElements.Select(s => s.ToString()).ToArray(), paramElements.Select(s => s.ToString()).ToArray(), returnsText.ToString(), remarksText.ToString(), exampleText.ToString(), valueText.ToString(), exception.Select(s => s.ToString()).ToArray());

return new DocumentationComment(summaryText.ToString(), typeParamElements.Select(s => s.ConvertToDocumentedObject()).ToArray(), paramElements.Select(s => s.ConvertToDocumentedObject()).ToArray(), returnsText.ToString(), remarksText.ToString(), exampleText.ToString(), valueText.ToString(), exception.Select(s => s.ConvertToDocumentedObject()).ToArray());
Copy link

Choose a reason for hiding this comment

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

Put each argument on its own line.

Copy link
Contributor

@DustinCampbell DustinCampbell left a comment

Choose a reason for hiding this comment

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

LGTM

@rchande rchande merged commit d358e11 into OmniSharp:master Dec 14, 2017
@akshita31 akshita31 deleted the typelookup_v2 branch January 12, 2018 21:34
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