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

proposal: x/tools/cmd/stringer: add flag to use inline comment text as value, if any #20483

Closed
mvdan opened this issue May 24, 2017 · 4 comments

Comments

@mvdan
Copy link
Member

mvdan commented May 24, 2017

stringer is great, but in my opinion it's limited to the characters that you can use as a name in a Go program.

To illustrate, imagine constants that define operators in a language. You can't define the add operator, +, with its own string as the name. You have to use a descriptive name like Add or ADD, that consists of characters that can form an identifier in a Go program.

This is what go/ast does: https://golang.org/src/go/token/token.go#L35

I have a similar situation in a package of mine. I wanted these token constants to implement fmt.Stringer, so I first set up a String() method that would just switch on the value and return a string.

This had a couple of major issues, however. For one, it was inefficient. The way stringer lays out the data in a single string and uses indices is much faster. For another, it was cumbersome to maintain that boilerplate code that was overly verbose.

So I laid out these values with inline comments containing only their string representation: https://github.com/mvdan/sh/blob/master/syntax/tokens.go

	and    // &
	andAnd // &&
	orOr   // ||
	or     // |
	orAnd  // |&

And I modified stringer (CL incoming) to add a flag to use an inline comment's text as the string value, rather than the name of the identifier itself. This of course falls back to the identifier name if there is no inline comment.

You can see the result in this file: https://github.com/mvdan/sh/blob/master/syntax/token_string.go

I realise this behaviour may not be desired in many situations, so that's why I think it's best to hide it behind a flag.

As said, CL incoming - the patch is rather simple.

@mvdan mvdan added the Proposal label May 24, 2017
@mvdan mvdan added this to the Proposal milestone May 24, 2017
@mvdan
Copy link
Member Author

mvdan commented May 24, 2017

@gopherbot seems a bit slow, so: https://go-review.googlesource.com/c/44076/

@rsc
Copy link
Contributor

rsc commented Jun 12, 2017

for @robpike

@mvdan
Copy link
Member Author

mvdan commented Jun 13, 2017

@robpike approved the proposal in the CL, marking as such.

@mvdan
Copy link
Member Author

mvdan commented Dec 20, 2017

The CL has been merged, but I botched the "fixes" line again. Closing manually.

@mvdan mvdan closed this as completed Dec 20, 2017
@golang golang locked and limited conversation to collaborators Dec 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants