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 DocService's docString to html view #174

Merged
merged 1 commit into from
Jun 3, 2016

Conversation

jongyeol
Copy link
Contributor

Motivation:

Current DocService's html view doesn't contain JavaDoc string.
This is second part of adding docstring to DocService.

Modifications:

Render DocString to html view if it exists.

Result:

DocService's html output contains DocString.

@jongyeol
Copy link
Contributor Author

This is an output example.

docstring-js-example

@anuraaga
Copy link
Collaborator

We currently use highlight.js to render json responses in debug page, and it seems that javascript rendering using it may produce more pleasing docstrings (@param etc would be highlighted).

What do you think about converting the raw docstrings into javadoc-ilke strings with the asterisks and using highlight.js to render?

Github renders docstring like this, I believe highlightjs would be the same.

/**
  * Basic unit of data within a ColumnFamily
  *
  * @param name, the name by which
  * @param value. The data associated
  */

@@ -88,6 +88,7 @@ <h2 class="sub-header">Parameters</h2>
<th>Name</th>
<th>Required</th>
<th>Type</th>
<th>DocString</th>
Copy link
Member

Choose a reason for hiding this comment

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

Description?

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

@trustin trustin self-assigned this May 25, 2016
@trustin trustin modified the milestone: 0.19.0.Final May 25, 2016
@jongyeol jongyeol force-pushed the feature/docservice-docstring-2-view branch from 9a8e72e to b145246 Compare May 26, 2016 13:03
@jongyeol
Copy link
Contributor Author

Added DocString to classes and functions too.

view2

@jongyeol
Copy link
Contributor Author

@anuraaga , Actually, asterisks were already removed when Thrift generated json output. Thrift's json output doesn't contain asterisks. Of course, I agree highlighted doc string is better, but I'm not sure adding asterisks is good or not. I checked highlight.js, but it has no javadoc only grammar. If we want to highlight, we need to add asterisks to descriptions.

@jongyeol
Copy link
Contributor Author

@trustin , Sorry to late. Updated it. And, it supports for functions and classes now.

@@ -189,6 +206,20 @@ $(function () {
});
}

function escapeHtml(string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it's time to add the "javascript standard library"? ;)

https://lodash.com/docs#escape

No worries either way

Copy link
Contributor Author

@jongyeol jongyeol Jun 3, 2016

Choose a reason for hiding this comment

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

It looks good, but I don't want to add another library for just one function. Maybe, we can add later if we need more.

@anuraaga
Copy link
Collaborator

Got it - anyways I think we can add this in a simple way for now and iterate on the look+feel. My feeling is that it might be worth round-tripping through add asterisks -> highlight -> remove asterisks (or maybe not) since the highlighting could help a lot, but it'd definitely need experimentation.

Mostly LGTM

@jongyeol jongyeol force-pushed the feature/docservice-docstring-2-view branch from b145246 to f06c6e2 Compare May 31, 2016 12:21
@jongyeol
Copy link
Contributor Author

@trustin , I extracted @params to fields' descriptions. Review again please~

class-docstring

@jongyeol jongyeol force-pushed the feature/docservice-docstring-2-view branch from f06c6e2 to fa5177d Compare May 31, 2016 12:28
@@ -118,6 +118,10 @@ label {
font-size: 60%;
}

.main div.description {
font-size: 100%;
Copy link
Member

Choose a reason for hiding this comment

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

How about adding: text-align: justify

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I'm not good at css. ;( BTW, I'll update it.

@trustin
Copy link
Member

trustin commented Jun 3, 2016

@jongyeol Would you mind pasting the updated screenshot one last time? Thanks for your patience!

@trustin trustin added this to the 0.19.1.Final milestone Jun 3, 2016
Motivation:

Current DocService's html view doesn't contain JavaDoc string.
This is second part of adding docstring to DocService.

Modifications:

Render DocString to html view if it exists.

Result:

DocService's html output contains DocString.
@jongyeol jongyeol force-pushed the feature/docservice-docstring-2-view branch from fa5177d to c640d7b Compare June 3, 2016 02:55
@jongyeol
Copy link
Contributor Author

jongyeol commented Jun 3, 2016

  • Services - Method

docstring-screenshot-service-function

  • Classes

docstring-screenshot-1-class

@jongyeol
Copy link
Contributor Author

jongyeol commented Jun 3, 2016

@trustin , I updated it with updating regexp (http://regexr.com/3di3d) and css things. :)

@trustin trustin merged commit d58e1f9 into line:master Jun 3, 2016
@trustin
Copy link
Member

trustin commented Jun 3, 2016

@jongyeol Merged!

@jongyeol jongyeol deleted the feature/docservice-docstring-2-view branch June 3, 2016 05:18
@jongyeol
Copy link
Contributor Author

jongyeol commented Jun 3, 2016

Thank you!

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.

3 participants