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

Support markdown when using @Description Annotation #4304

Merged
merged 58 commits into from
Jul 29, 2022

Conversation

cnabro
Copy link
Contributor

@cnabro cnabro commented Jun 18, 2022

Motivation:

  • Support markdown when using @description Annotation
  • Armeria DocService can be extended and used for various purposes

Modifications:

<img width=1681" alt="image" src="https://user-images.githubusercontent.com/6916639/174469675-daa21c8a-6fd3-40c0-bd78-0c538bdfcf0c.png">
image
image

Result:

@CLAassistant
Copy link

CLAassistant commented Jun 18, 2022

CLA assistant check
All committers have signed the CLA.

@cnabro cnabro marked this pull request as draft June 18, 2022 18:03
@cnabro cnabro force-pushed the feature/support-markdown-docservice branch 3 times, most recently from 0c273bf to 6e39005 Compare June 18, 2022 18:37
@codecov
Copy link

codecov bot commented Jun 18, 2022

Codecov Report

Merging #4304 (5e465ff) into master (8b0225e) will decrease coverage by 0.05%.
The diff coverage is 75.00%.

@@             Coverage Diff              @@
##             master    #4304      +/-   ##
============================================
- Coverage     73.57%   73.52%   -0.06%     
- Complexity    17654    17665      +11     
============================================
  Files          1500     1504       +4     
  Lines         65991    66094     +103     
  Branches       8325     8335      +10     
============================================
+ Hits          48555    48597      +42     
- Misses        13236    13287      +51     
- Partials       4200     4210      +10     
Impacted Files Coverage Δ
...linecorp/armeria/server/docs/DocServicePlugin.java 20.00% <ø> (ø)
...om/linecorp/armeria/server/docs/NamedTypeInfo.java 100.00% <ø> (ø)
...ver/annotated/kotlin/MarkdownDescriptionService.kt 29.41% <29.41%> (ø)
...nal/server/annotation/AnnotatedServiceFactory.java 92.12% <40.00%> (-0.74%) ⬇️
...va/com/linecorp/armeria/server/docs/FieldInfo.java 60.52% <50.00%> (-3.37%) ⬇️
...ava/com/linecorp/armeria/server/docs/EnumInfo.java 37.93% <60.00%> (ø)
.../linecorp/armeria/server/docs/DescriptionInfo.java 66.66% <66.66%> (ø)
...linecorp/armeria/server/docs/FieldInfoBuilder.java 70.00% <66.66%> (ø)
...rver/annotated/kotlin/MermaidDescriptionService.kt 66.66% <66.66%> (ø)
...l/server/annotation/AnnotatedDocServicePlugin.java 82.47% <68.42%> (-0.94%) ⬇️
... and 52 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b0225e...5e465ff. Read the comment docs.

@cnabro cnabro force-pushed the feature/support-markdown-docservice branch from 4e288f2 to b76cd63 Compare June 19, 2022 07:01
@cnabro cnabro marked this pull request as ready for review June 19, 2022 14:47
Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

This looks really amazing. Thanks a lot, @cnabro! 😄
Do you mind adding test cases of ResponseObject and Description to Java test class especially to AnnotatedDocServiceTest?
We use the class as a playground of DocService using the env value (DOC_SERVICE_DEMO) so it would be very helpful if we can use it there. 😄

@minwoox minwoox added this to the 1.17.0 milestone Jun 22, 2022
@cnabro
Copy link
Contributor Author

cnabro commented Jun 22, 2022

@minwoox ok, i'll try it. Thanks for the review ! 👍

@cnabro
Copy link
Contributor Author

cnabro commented Jun 23, 2022

As mentioned in Slack,
Rollback @ResponseObject for #4309. And I added some test-case for AnnotatedDocServiceTest.

@minwoox @ikhoon

@cnabro cnabro force-pushed the feature/support-markdown-docservice branch from 92163ad to e89486d Compare June 23, 2022 09:58
@minwoox
Copy link
Member

minwoox commented Jun 23, 2022

image

I found out that the markdown String is not properly rendered when the page has a scroll. We need to fix that or we can probably just disable markdown for the parameter and struct description.

Do you think it's worth supporting markdown in the parameter description as well?

@cnabro
Copy link
Contributor Author

cnabro commented Jun 23, 2022

@minwoox I think worth supporting markdown in the parameter description as well 😅
It's easy to express the specification like Deprecated,removed or See this link and etc..

@cnabro
Copy link
Contributor Author

cnabro commented Jun 23, 2022

And I found the same issue on previous version (on disabled markdown). I think it's not a markdown issue.
image

I think the workaround is not use table, or customize <TableCell> for resizing.
(There is so much to express in table 🐤)

Markdown is already support resizing.
image

@minwoox
Copy link
Member

minwoox commented Jun 24, 2022

And I found the same issue on previous version (on disabled markdown). I think it's not a markdown issue

Oops, I didn't know that we had such a bug. 😓

I think the workaround is not use table

Then which one do you want to use instead? Could you tell me more about this, please? 😄

@cnabro
Copy link
Contributor Author

cnabro commented Jun 24, 2022

@minwoox
After write comments, I found out some solution.
Wrapping <Table> with <TableContainer> will be adjust overflowX

https://github.com/mui/material-ui/blob/v4.9.0/packages/material-ui/src/TableContainer/TableContainer.js#L10

<TableContainer> // wrapping table
        <Table>
        <TableHead>
          <TableRow>
            <TableCell>Name</TableCell>
            {hasLocation && <TableCell>Location</TableCell>}
            <TableCell>Required</TableCell>
            <TableCell>Type</TableCell>
            <TableCell>Description</TableCell>
            {hasBean && <TableCell />}
          </TableRow>
        </TableHead>
        <TableBody>
          <FieldInfos
            hasLocation={hasLocation}
            indent={0}
            title={title}
            variables={variables}
            specification={specification}
          />
        </TableBody>
      </Table>
      </TableContainer>

How about fixing the issue like this screenshot using scroll?
image

@minwoox
Copy link
Member

minwoox commented Jun 24, 2022

How about fixing the issue like this screenshot using scroll?

That looks awesome. 😆

@minwoox
Copy link
Member

minwoox commented Jun 24, 2022

Had a chat with @trustin and how about adding a metadata to @Description so that we can support various markup such as Mermaid.js?

@Description(value= "...", markup = Markup.NONE)
@Description(value= "...", markup = Markup.MARKDOWN)
@Description(value= "...", markup = Markup.MERMAID) // we can add this later.

@cnabro
Copy link
Contributor Author

cnabro commented Jun 24, 2022

@minwoox Good idea. It will be used for various purposes. 👍
I'll try to add a metadata for @Description

@cnabro cnabro marked this pull request as draft June 24, 2022 14:09
@cnabro cnabro force-pushed the feature/support-markdown-docservice branch 2 times, most recently from aa1edeb to ea2e4f6 Compare June 24, 2022 17:07
cnabro and others added 15 commits July 20, 2022 10:23
…tion/AnnotatedDocServicePlugin.java

Co-authored-by: Ikhun Um <[email protected]>
…tion/AnnotatedDocServicePlugin.java

Co-authored-by: Ikhun Um <[email protected]>
…tion/AnnotatedDocServiceTest.java

Co-authored-by: Ikhun Um <[email protected]>
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Awesome, @cnabro! 🚀💯
As you know, documentation service is the most beloved feature.
Many Armeria users will be happy with this.

@@ -145,7 +147,7 @@ public ServiceSpecification generateSpecification(Set<ServiceConfig> serviceConf
return generate(serviceDescription, methodInfos);
}

private static void addServiceDescription(Map<Class<?>, String> serviceDescription,
private static void addServiceDescription(Map<Class<?>, DescriptionInfo> serviceDescription,
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: This method will be changed by #4322.
I'd like to ask @cnabro for a review after they are merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. I'll wait for it 👍

@ikhoon ikhoon merged commit f01b02b into line:master Jul 29, 2022
@ikhoon
Copy link
Contributor

ikhoon commented Jul 29, 2022

Thanks a lot for your hard work! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants