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

Docs: ApiDocsGenerator for API documentation from XML comments #5846

Merged
merged 87 commits into from
Dec 10, 2024

Conversation

tesar-tech
Copy link
Collaborator

Description

  • Introduced a new source generator project, ApiDocsGenerator, to automatically extract public properties with the [Parameter] attribute from Blazorise components.
  • The generator produces a source file (ComponentsApiDocsSource) containing a dictionary to retrieve all parameters for a given component.
  • Usage Example: In ButtonPage.razor:
<ComponentApiDocs ComponentType="typeof(Button)" />

This single line will generate the API documentation at the bottom of the page, populated with XML comments.

TODOs

  1. Verify XML Comments:
    Ensure the XML comments accurately represent what we want to display publicly. While this is generally true, there are discrepancies—for example, the comments for the Button component differ a bit (parameter Active)

  2. Default Values Handling:

    • The current implementation only displays default values for basic types but not for Blazorise enums or complex types.
    • Defaults like [Parameter] public int SomeParam { get; set; } = 5; are not respected yet.
    • These cases could likely be addressed within the source generator but how "far"?
  3. Type Differences:

    • For instance, Button.Clicked is now EventCallback<MouseEventArgs> but was previously just EventCallback.
    • Determine the standard format we want to display for such types.
  4. Adding Extensions:

    • The same logic can be extended to other projects, but it will need to be configured for each .csproj file individually.
  5. Handling Special Cases:

    • For example, DataGrid includes DataGridColumn. Currently, column properties appear in the DataGrid API page.
    • Decide whether this behavior is correct or if such properties should be separated.
  6. Clean up the actual source generator

  • Currently all that is in one file, no comments, etc.. Will be done, once feature requirements are established.

@tesar-tech tesar-tech requested a review from stsrki November 15, 2024 17:56
@stsrki
Copy link
Collaborator

stsrki commented Nov 15, 2024

  1. Verify XML Comments:
    Ensure the XML comments accurately represent what we want to display publicly. While this is generally true, there are discrepancies—for example, the comments for the Button component differ a bit (parameter Active)

This is something that we will need to handle from the code side. We need to rewrite comments so that they are properly formated and dramatically correct. Most comments are quite old and are not as good as they could be.

  1. Default Values Handling:

    • The current implementation only displays default values for basic types but not for Blazorise enums or complex types.
    • Defaults like [Parameter] public int SomeParam { get; set; } = 5; are not respected yet.
    • These cases could likely be addressed within the source generator but how "far"?

I see. Some defaults are only set in the code further. We have two options. Define default on the property, or mention it in the comment.

  1. Type Differences:

    • For instance, Button.Clicked is now EventCallback<MouseEventArgs> but was previously just EventCallback.
    • Determine the standard format we want to display for such types.

The real type should be correct, so EventCallback<MouseEventArgs>.

  1. Adding Extensions:

    • The same logic can be extended to other projects, but it will need to be configured for each .csproj file individually.

Makes sense: Question! Does this new generator need to be referenced as part of the nuget once we publish blazorise?

  1. Handling Special Cases:

    • For example, DataGrid includes DataGridColumn. Currently, column properties appear in the DataGrid API page.
    • Decide whether this behavior is correct or if such properties should be separated.

I think whichever "type of (ComponentName)" is defined, we need to list it. So maybe ComponentType can be an array?

  1. Clean up the actual source generator
  • Currently all that is in one file, no comments, etc.. Will be done, once feature requirements are established.

@tesar-tech
Copy link
Collaborator Author

  1. Default Values Handling:

    • The current implementation only displays default values for basic types but not for Blazorise enums or complex types.
    • Defaults like [Parameter] public int SomeParam { get; set; } = 5; are not respected yet.
    • These cases could likely be addressed within the source generator but how "far"?

I see. Some defaults are only set in the code further. We have two options. Define default on the property, or mention it in the comment.

Option 1 sounds better - one source of truth. If not doable by property we can agree on some format for handling it from comment. Like: //__default__ stringOfSomeDefaultValue , which will be processed and put into Default column (and removed from comment)

  1. Adding Extensions:

    • The same logic can be extended to other projects, but it will need to be configured for each .csproj file individually.

Makes sense: Question! Does this new generator need to be referenced as part of the nuget once we publish blazorise?

Should be handled by PrivateAssets="all" in the csproj -> doesn't get to the nuget package.

  1. Handling Special Cases:

    • For example, DataGrid includes DataGridColumn. Currently, column properties appear in the DataGrid API page.
    • Decide whether this behavior is correct or if such properties should be separated.

I think whichever "type of (ComponentName)" is defined, we need to list it. So maybe ComponentType can be an array?

This gonna be more complicated, Extensions are wild. We don't want just the [Parameter]s to be part of the docs. We also need JSOptions in some cases (Charts), etc... Maybe we'll need some customizations for individual Extensions. I would do the Components first and then look into the Extensions.

@tesar-tech
Copy link
Collaborator Author

I focused on default values. From what I have checked, they should all work.

Default values

[Parameter] public string SomeValue { get; set; } = "some string value";

It also handles cases where the default value is not a constant.

[Parameter] public TimeSpan Interval { get; set; } = TimeSpan.FromSeconds(10);

Additionally, it works when the default value is set by a backing field.

private string _backingField = "backed value";
[Parameter] public string BackedValue 
{
    get => _backingField;
    set => _backingField = value;
}

Moreover, it preserves the exact expression of default values, not just the computed result.

[Parameter] public int ComplexValue { get; set; } = 20 * 200; // Output: "20 * 200", not "4000"

Enum values for Blazorise enums..

I added the listing of blazorise enums:

image

the text Possible values: Button, Submit, Reset, Link is not part of the comment..

I didn't persuade further, because I figured there is this Enumeration record, which is kind of an enum, but it's bit harder to obtain the values (not impossible) from.

Docs pages can handle multiple components..

Component and its related components:

<ComponentApiDocs ComponentTypes="[typeof(Accordion), typeof(AccordionToggle), typeof(AccordionItem), typeof(AccordionHeader), typeof(AccordionBody)]" />

Will focus on methods now.

@stsrki
Copy link
Collaborator

stsrki commented Nov 20, 2024

I have noticed we will have some edge cases with inherited parameters. For example TextEdit is inherited from BaseTextInput, and then from BaseInputComponent. We need to show all of those parameters. But, I would not go all the way up to BaseComponent.

For BaseComponent I think it is good to have a separate API page in the docs and then reference it with a link. Every Blazorise component is inherited from it, and it would be too much to list all of its parameters for each component.

@tesar-tech
Copy link
Collaborator Author

I have noticed we will have some edge cases with inherited parameters. For example TextEdit is inherited from BaseTextInput, and then from BaseInputComponent. We need to show all of those parameters. But, I would not go all the way up to BaseComponent.

What about just bringing it within the list:

<ComponentApiDocs ComponentTypes="[typeof(TextEdit), typeof(BaseTextInput)]" />

It will be separated from each other... TextEditPage has this now. I have added the functionality for generic types (like BaseTextInput).

Or we I can do something like:

<ComponentApiDocs JoinedComponentTypes=[ [typeof(TextEdit), typeof(BaseTextInput)] ] />

For BaseComponent I think it is good to have a separate API page in the docs and then reference it with a link. Every Blazorise component is inherited from it, and it would be too much to list all of its parameters for each component.

I agree. We can create the page manually and add the link to the ComponentApiDocs

I just need to add base class to the sg.

@stsrki
Copy link
Collaborator

stsrki commented Nov 21, 2024

I have noticed we will have some edge cases with inherited parameters. For example TextEdit is inherited from BaseTextInput, and then from BaseInputComponent. We need to show all of those parameters. But, I would not go all the way up to BaseComponent.

What about just bringing it within the list:

<ComponentApiDocs ComponentTypes="[typeof(TextEdit), typeof(BaseTextInput)]" />

It will be separated from each other... TextEditPage has this now. I have added the functionality for generic types (like BaseTextInput).

Or we I can do something like:

<ComponentApiDocs JoinedComponentTypes=[ [typeof(TextEdit), typeof(BaseTextInput)] ] />

For BaseComponent I think it is good to have a separate API page in the docs and then reference it with a link. Every Blazorise component is inherited from it, and it would be too much to list all of its parameters for each component.

I agree. We can create the page manually and add the link to the ComponentApiDocs

I just need to add base class to the sg.

Having them separate is not good because user should not care how it is structured internally. They only care that TextEdi has the parameters needed. And TextEdit can only be used as an external API.

The second option with JoinedComponentTypes might be better. But I don't like it mainly because we then need to take care of all hierarchy. Better to do it with the SC now that we are committed to it. There are going to be some rules, yes, but in the end it is a lot easier for later for us.

@tesar-tech
Copy link
Collaborator Author

Better to do it with the SC now that we are committed to it. There are going to be some rules, yes, but in the end it is a lot easier for later for us.

Agree. I can define these "joins" in the SG. "Every component that inherits from BaseTextInput lists also its params".

Are there any other such "joins" or "exception" to what we already have?

@stsrki
Copy link
Collaborator

stsrki commented Nov 21, 2024

Better to do it with the SC now that we are committed to it. There are going to be some rules, yes, but in the end it is a lot easier for later for us.

Agree. I can define these "joins" in the SG. "Every component that inherits from BaseTextInput lists also its params".

Are there any other such "joins" or "exception" to what we already have?

I would join everything that goes up to BaseComponent. Everything in BaseComponent should be excluded and optionally documented somewhere else.

@tesar-tech
Copy link
Collaborator Author

I would join everything that goes up to BaseComponent. Everything in BaseComponent should be excluded and optionally documented somewhere else.

  • Now it does exactly that.
  • Now it works for extensions too. The same way. The only thing that's needed is to reference the SG project and it will bring the documentation into ComponentsApiDocsSource (currently done for Snackbar`)
  • It needs some refactoring, cleanups, comments. Will be done, once we finalize the functionality.

"Issues" with inheritance

  • The public methods - it scans also for SetParametersAsync, which is also on two places (TextEdit and BaseInputComponent).
  • Virtual methods - it also brings the virtual methods, not sure if it is right or not..
  • It doesn't process the <inheritdoc/> xml comment.

So question is "How to deal with the inheritance of methods and their comments"?

Also you mention some black-listed methods, what are those?

@stsrki
Copy link
Collaborator

stsrki commented Nov 22, 2024

Also you mention some black-listed methods, what are those?

Any public method that is only meant to be used internally. For example BeginAnimation on Modal. Or, SetValue on NumericPicker. As mentioned before, we could remove it by adding the "Should only be used internally" comment. Which lead us to the second problem.

So question is "How to deal with the inheritance of methods and their comments"?

I can only think that we need to read XML comments file to get this information.

@stsrki
Copy link
Collaborator

stsrki commented Nov 22, 2024

BTW, my original idea was to generate API docs source code as a *Api.razor component in Blazorise.Docs project. Not to generate it as a ComponentsApiDocsSource.

By generating it directly into the razor files, we can immediately see what is being generated. And we don't need to run the application to see the changes. Making it easier to review what needs to be omitted by extra rules.

@tesar-tech
Copy link
Collaborator Author

Also you mention some black-listed methods, what are those?

Any public method that is only meant to be used internally. For example BeginAnimation on Modal. Or, SetValue on NumericPicker. As mentioned before, we could remove it by adding the "Should only be used internally" comment. Which lead us to the second problem.

So question is "How to deal with the inheritance of methods and their comments"?

I can only think that we need to read XML comments file to get this information.

What about [NotInApiDocs] parameter? It's more straightforward than looking for a string.

What about the "duplicates" like OnParameterSet ? You want them both in there? Or just the one from the top-parent (and its comment?)


## System.Runtime

Retrieving XML comments from the `System.Runtime` assembly can be problematic, particularly for the `IDisposable` interface.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you tried this library to read XML comments? Maybe they have already solved this kind of problems?

https://github.com/loxsmoke/DocXml

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The issue I am facing with System.Runtime is that it doesn't appear to the analyzer that IDispose is from System.Runtime. Somehow, it's blind to this assembly, so it "doesn't know which XML to load".

@stsrki
Copy link
Collaborator

stsrki commented Dec 9, 2024

Major Problem
Some components include (JS)options in their documentation, but the ApiDocs generator is not designed to scan these. It only processes components. Affected extensions include: Captcha, Chart, Cropper, (possibly more)

We define every API docs explicitly by typeof(ComponentName). So by that we should be able to read whatever we want. We don't need to scan entire assembly. The only difference from regular class(JS options), and component is the inheritance. Components go to BaseComponent/ComponentBase, and classes are read as they are, in full.


Minor Problems

  • Captcha: The State property does not appear in ApiDocs because it is not marked as a [Parameter]. However, it is documented in the original source.

Possibly and overlook from our side.

  • RouterTabsPageAttribute: This is not a component, so it does not have a record in ApiDocs. This is similar to the "options" problem.

This should be same as my previous comment.

  • Validation: The Validate method uses <inheritdoc>, but there is no valid source to inherit from. This is not the only instance of invalid <inheritdoc> usage.

We should fix this kind of problem. But in another PR. Once we merger this PR. We are going to improve comments. You can just make a list in an issue so that we know what to do.

  • Chart: Many XML comments are missing.

Same, we will fix it later.

  • Icon: The documentation page for the Icon is in the Extensions, but the Icon itself is defined in Blazorise.

Because Icon cannot be used without an extension, just define typeof(Icon) from Blazorise core.

@stsrki
Copy link
Collaborator

stsrki commented Dec 9, 2024

For options classes, ie ReCaptchaOptions, we can leave them as they are now in the docs. And only focus on component SG.

@stsrki
Copy link
Collaborator

stsrki commented Dec 9, 2024

However, it was unable to inherit XML comments from System.Runtime.IDisposable.Dispose. This issue (pobably) arises because System.Runtime is an implicit reference (but I didn't dig too deep). As a result, <inheritdoc> cannot resolve the comment for Dispose.

  • Possible solution: Avoid using <inheritdoc> for such cases, as Dispose is typically overridden to add functionality beyond simple disposal.

Make a list of methods to ignore methods, and add Dispose onto it. The user should not be aware of it.

@stsrki
Copy link
Collaborator

stsrki commented Dec 9, 2024

@tesar-tech you can also ignore all internal components. Every component that starts with _ is considered internal. This is our naming convention.

image

@stsrki
Copy link
Collaborator

stsrki commented Dec 9, 2024

image

For enums like this, we could omit Blazorise.Video. namespace and write it as VideoControlsType.PlayLarge, VideoControlsType.Play, etc.

@stsrki
Copy link
Collaborator

stsrki commented Dec 9, 2024

For Icon possible values, this seems like an overkill. Maybe for Icon we don't show possible values? Or if we find any enum that has more than some N possible values?

image

@stsrki
Copy link
Collaborator

stsrki commented Dec 9, 2024

image

This stopped working.

@tesar-tech
Copy link
Collaborator Author

This stopped working.

Just the generic type were not handled correctly. Fixed.

For Icon possible values, this seems like an overkill. Maybe for Icon we don't show possible values? Or if we find any enum that has more than some N possible values?

What about displaying just first 5 values and then button Show all similar to how Remarks is handled?

@tesar-tech you can also ignore all internal components. Every component that starts with _ is considered internal. This is our naming convention.

I moved the code little bit ahead, so it doesn't get processed at all. Also all the conditions are now on one place within one switch expression.

  ( bool continueProcessing, bool skipParamAndComponentCheck ) = type switch
        {
            _ when type.TypeKind != TypeKind.Class || type.DeclaredAccessibility != Accessibility.Public  => (false, false),
            _ when type.Name.StartsWith( '_' ) => ( false, false ),
            _ when type.Name.EndsWith( "Options" ) => ( true, true ),
            _ when type.Name.EndsWith( "RouterTabsPageAttribute" ) => ( true, true ),
            _ when !type.AllInterfaces.Any( i => i.Name == "IComponent" ) => ( false, false ),
            _ => ( true, false )
        };

Which also solves the RouterTabsPageAttribute issue and is one line of code to also add the Options. So it's there and you can use them. Not sure if you want to still omit it, so only Cropper has that as demonstration.

<ComponentApiDocs ComponentTypes="[typeof(Cropper),typeof(CropperViewer), typeof(CropperCropOptions)]" />

It would be maybe worth it to add them under Options heading (on the same level as Methods, Params, Events)...?

For enums like this, we could omit Blazorise.Video. namespace and write it as VideoControlsType.PlayLarge, VideoControlsType.Play, etc.

Little bit hacky, but fixed.

@stsrki
Copy link
Collaborator

stsrki commented Dec 10, 2024

What about displaying just first 5 values and then button Show all similar to how Remarks is handled?

It might work but don't worry about it now.

We can also make an exception and add a URL to the Icons page docs/extensions/icons-available.

For enums like this, we could omit Blazorise.Video. namespace and write it as VideoControlsType.PlayLarge, VideoControlsType.Play, etc.

Little bit hacky, but fixed.

Nice :)


We are about done for now. I want to move from this to other tasks. We can always improve documentation later. If anything goes wrong, we will fix it.

Just to have a note for later. Instead of generating Blazorise.ApiDocs.cs, and other files. We can generate ComponentNameApi.razor with real Parameters, Events, and Methods section. What are the benefits? Well, for a start, it would be a lot faster in real time because we will not need to iterate for type of (ComponentName) and find each parameter, etc. Everything would already be precompiled. We will do that for 2.0.

@stsrki stsrki merged commit 4bba734 into rel-1.7 Dec 10, 2024
2 checks passed
@stsrki stsrki deleted the sup/rel-1.7/xml-comments-autogen branch December 10, 2024 09:11
@github-actions github-actions bot locked and limited conversation to collaborators Dec 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants