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

Merge branch 'feat/method_summary' #300

Closed
wants to merge 2 commits into from
Closed

Merge branch 'feat/method_summary' #300

wants to merge 2 commits into from

Conversation

ppaanngggg
Copy link
Contributor

Use the first line of operation comments as summary, for better format in document.

@ppaanngggg ppaanngggg requested a review from a team as a code owner January 23, 2022 05:28
@ppaanngggg
Copy link
Contributor Author

oh, if I want to pass the unit tests, I have to modify the groundtruth by adding summary for methods.

@ppaanngggg
Copy link
Contributor Author

oh, if I want to pass the unit tests, I have to modify the groundtruth by adding summary for methods.

@timburks Should I modify the groundtruth?

@morphar
Copy link
Collaborator

morphar commented Feb 9, 2022

This is just my opinion:
I'm not sure, this is a good idea or even in line with the general philosophy of the project.
In general, the proto definitions are converted to OpenAPI, in a way that it can be converted back.
Decision on naming are based on the ability to relate it directly back to the original proto definition.

Also: by manipulating the comment you may loose data in an unexpected way.
E.g. if you use a proto linter, it may split you comment like this:

  // This is a comment that reached beyond 80 characters, so this will be split in 2

Will become:

  // This is a comment that reached beyond 80 characters, so this will be split
  // in 2

I also really appreciate the fact, that I can focus my attention on the proro files, their naming and structure and let gnostic generate the rest, without having to consider what it does, because it just converts between 2 formats, without changing things more than absolutely necessary.

@timburks what are your thoughts on this?

@ppaanngggg
Copy link
Contributor Author

Also: by manipulating the comment you may loose data in an unexpected way.

I see, my idea is too tricky.

If you finally decide to discard it, close this PR.

Thx

@alexeyten
Copy link

alexeyten commented Feb 9, 2022

Is there anything like jsdoc tags? I mean could we annotate summary like

service TestService {
  // @summary Short summary
  //
  // Long description
  // Lorem...
  rpc Get (MessageIn) returns (MessageOut) {
    ...
  }

Or could we add an option generate_summary=true?

@morphar
Copy link
Collaborator

morphar commented Feb 11, 2022

I'm not sure, this is within scope of gnostic.
gnostic takes a pretty clean approach to converting between types and is only opinionated where it needs to.
Adding jsdoc string parsing would open up all kinds of new issues, like parsing all kinds of doc string parsing.
Correct me if I'm wrong @timburks :)

My approach in these situations is to let gnostic do what it's good at and then have a step more in my personal build process, where I read the OpenAPI file, change what I need and then save the file with the changes.

@ppaanngggg
Copy link
Contributor Author

My approach in these situations is to let gnostic do what it's good at and then have a step more in my personal build process, where I read the OpenAPI file, change what I need and then save the file with the changes.

Great idea. I will close it and keep this little modify in my special branch for convenience.

@ppaanngggg ppaanngggg closed this Feb 11, 2022
@ppaanngggg ppaanngggg deleted the google/feat/operation_summary branch March 25, 2022 08:15
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