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

don't keep trailing newline inside line comment txt in AST #48

Merged
merged 9 commits into from
Sep 26, 2022

Conversation

jhump
Copy link
Member

@jhump jhump commented Sep 26, 2022

This is in a similar vein as #46: protoc considers the trailing newline for line comments as part of the comment text in its source code info.

But that is confusing for users of the AST because the AST ostensibly provides other accessors for querying whitespace between elements: LeadingWhitespace() method on NodeInfo and Comment. By keeping the newline in the comment text, that means that code has to look at both the LeadingWhitespace() and also inspect the preceding comment's RawText() to determine if there was a newline between elements.

That's icky and is the source of some confusing and brittle code in the formatter. We can simplify by keeping the trailing/separating newline out of the comment text, so it is instead part of leading whitespace.

However, we have to remember to then add the newline on the end when generating source code info, since protoc includes it.

While making this change, I refactored a decent bit inside the AST package's file info. It now uses generics to model a sequence of items or tokens. An item is a token or a comment. One neat thing this provides is a simple API for navigating the parsed file as a stream of tokens (or a stream of all items, for consumers that want to inspect the comments, too), instead of needing to use a recursive approach over the tree structure.

@jhump jhump requested a review from pkwarren September 26, 2022 14:23
ast/file_info.go Outdated Show resolved Hide resolved
ast/file_info.go Outdated Show resolved Hide resolved
sourceinfo/source_code_info.go Outdated Show resolved Hide resolved
sourceinfo/source_code_info.go Outdated Show resolved Hide resolved
@jhump
Copy link
Member Author

jhump commented Sep 26, 2022

@pkwarren, I also just pushed a commit with new tests that are small unit tests for the two Sequence implementations -- to compare the sequences to the data from walking the tree.

@jhump jhump merged commit 3b93514 into main Sep 26, 2022
@jhump jhump deleted the jh/ast-line-comment-should-not-include-newline branch September 26, 2022 18:25
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.

2 participants