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

Comments associated with Visitess when newlines between the comment and Visitee #26

Closed
bufdev opened this issue Dec 6, 2017 · 6 comments

Comments

@bufdev
Copy link
Contributor

bufdev commented Dec 6, 2017

Example:

// foo

// bar

syntax = "proto3";

// baz

package bat;

// bar is the Comment on the Syntax object, and // baz is the Comment on the Package object. // foo correctly is associated with just the Proto object and will be hit when you get to VisitComment on Proto.

@emicklei
Copy link
Owner

This is not how the package works and thought of. Empty lines will disassociate the comment. Without this, many of our definitions will have wrong format ; e.g. syntax will have the copyright as its comment. Field comment will dangle.
Maybe this behavior can be made optional.

@bufdev
Copy link
Contributor Author

bufdev commented Dec 11, 2017

This is how it is currently parsed, is what I am saying. Putting a newline does not disassociate the comment. In the above example, // bar will be the Comment field on Syntax, and // baz will be the Comment on Package. This is a bug if this is not how it should operate

@emicklei
Copy link
Owner

ok, i will review it again

@bufdev
Copy link
Contributor Author

bufdev commented Jan 2, 2018

Hi, any status on this?

@emicklei
Copy link
Owner

emicklei commented Jan 2, 2018

ic, the comments should not be associated because they are separated. I created a test for this to fix.

@bufdev
Copy link
Contributor Author

bufdev commented Jan 2, 2018

Your change here introduced a bug. Comments no longer appropriately associate.

$ cat yarpcproto/yarpc.proto
syntax = "proto3";

package uber.yarpc;

option go_package = "yarpcproto";

// Oneway is the return type to use for an rpc method if
// the method should be generated as oneway.
message Oneway {
  bool ack = 1;
}

The comment on Oneway is not associated with the *proto.Message.

Can you please revert?

emicklei pushed a commit that referenced this issue Jan 3, 2018
@bufdev bufdev mentioned this issue Jan 4, 2018
@emicklei emicklei closed this as completed Feb 2, 2018
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

No branches or pull requests

2 participants