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

Parse trailing comments in protoc-gen-openapiv2 #2929

Closed
ionling opened this issue Oct 12, 2022 · 2 comments · Fixed by #2965
Closed

Parse trailing comments in protoc-gen-openapiv2 #2929

ionling opened this issue Oct 12, 2022 · 2 comments · Fixed by #2965

Comments

@ionling
Copy link
Contributor

ionling commented Oct 12, 2022

comments := ""
if loc.LeadingComments != nil {
comments = strings.TrimRight(*loc.LeadingComments, "\n")
comments = strings.TrimSpace(comments)
// TODO(ivucica): this is a hack to fix "// " being interpreted as "//".
// perhaps we should:
// - split by \n
// - determine if every (but first and last) line begins with " "
// - trim every line only if that is the case
// - join by \n
comments = strings.Replace(comments, "\n ", "\n", -1)
}
return comments

The current protoc-gen-openapiv2 only parse leading comments, no trailing.
And I think trailing is better for short comments, like this:

message SearchRequest {
  string query = 1;              // Query string
  int32 page_number = 2;         // Page number
  int32 result_per_page = 3;     // Per page result count
}

instead of:

message SearchRequest {
  // Query string
  string query = 1;
  // Page number
  int32 page_number = 2;
  // Per page result count
  int32 result_per_page = 3;
}

So how about add this feature?

@johanbrandhorst
Copy link
Collaborator

Sure, sounds reasonable. What should the behavior be if there are both leading and trailing comments? I'm guessing we could just append them? Would you be willing to contribute a fix for this?

@ionling
Copy link
Contributor Author

ionling commented Oct 12, 2022

No problem, I'll submit a PR when I'm free.

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 a pull request may close this issue.

2 participants