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

Add support for generating the method and generic method comment signature with nested types #749

Closed

Conversation

hiihellox10
Copy link

@hiihellox10 hiihellox10 commented Apr 21, 2021

Hi @jbevain, current when a method or generic method with a nested type argument and that is defined in a generic class, the DocCommentId can't generate the comment signature of the method correctly. I created a few test methods with some special arguments for generate test XML documentation and make the DocCommentId can generate the same comment signature.

I don't have the specifications document of XML documentation that I now just found a basic introduction in Microsoft Docs but the introduction hasn't described the rules of the generic method. Although the comment signature of the generic method looks like has been fixed and maybe it's not a standard implementation. If you have the specifications document could you share them with me?

@hiihellox10 hiihellox10 changed the title Add support for the method and generic method with the nested type parameter in XML documentation Add support for generating the method and generic method comment signature with nested types defined in a generic class Apr 21, 2021
@jbevain
Copy link
Owner

jbevain commented Apr 23, 2021

Hey @hiihellox10, thanks for the PR!
Hey @joelmartinez, I know you depend on this, could you review?

@hiihellox10 hiihellox10 changed the title Add support for generating the method and generic method comment signature with nested types defined in a generic class Add support for generating the method and generic method comment signature with nested types Apr 26, 2021
Copy link
Contributor

@joelmartinez joelmartinez left a comment

Choose a reason for hiding this comment

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

Yes indeed ... this one was a long time coming. This looks great to me ... the added tests look like they match the generated IDs on the test classes.

@jbevain, it hasn't run the CI workflows. Not sure if this review will allow those to run, but if not I'd like to see the CI run, with your approval :)

@joelmartinez
Copy link
Contributor

Oh, also ... just for the sake of referencing, this is a fix to #606

@saucecontrol
Copy link

If you have the specifications document could you share them with me?

The spec Roslyn uses can be found here, and the implementation is here and here.

Note that different compilers (e.g. Roslyn vs MSVC) and doc processing tools (e.g. whatever MS uses for the docs that ship with the framework) can differ in their documentation ID syntax, so there may be more than one "correct" ID for a given type/member. ex: dotnet/roslyn#52599

@joelmartinez
Copy link
Contributor

@saucecontrol "whatever ms uses for the docs" is mdoc (which uses cecil's implementation). This is the reason that @hiihellox10 and I are working on this issue 🤣

Thanks for that link though, that was helpful. I think that reading through it (albeit briefly) the changes here seem to match the specs.

@saucecontrol
Copy link

Well that is fascinating. I happened to find this PR while looking up the source for GenericInstanceType to make sure I was navigating it correctly while re-working my own doc tool to allow for multiple doc ID syntaxes to account for these differences. Small world. 🤣

@hiihellox10
Copy link
Author

Hi @jbevain @joelmartinez, could you please help review this PR? Also, the CI is awaiting an approval.


int GetGenericTypeParameterCount (TypeReference type)
{
var returnValue = 0;
Copy link
Owner

Choose a reason for hiding this comment

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

Doesn't type.GenericParameters.Count work here?

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.

4 participants