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

x/tools/cmd/stringer: add a -trimprefix flag #16539

Closed
adrianduke opened this Issue Jul 29, 2016 · 14 comments

Comments

Projects
None yet
9 participants
@adrianduke

adrianduke commented Jul 29, 2016

Context

When writing constants its common to add a prefix to aid IDE auto-complete and also to explicitly group together common constants (see: net/http, net/http, crypto/tls, net/http/fcgi & net/interface).

Problem

x/tools/cmd/stringer generates a String() method with only the exact constant name:

//go:generate stringer -type=Type
type Type uint8

const (
    TypeUnknown Type = iota
    TypePayPal
    TypeAmazon
    TypeWorldPay
)

Would result in:

fmt.Println(TypeUnknown.String())
// out: TypeUnknown

fmt.Println(TypePayPal.String())
// out: TypePayPal

Whilst this is desirable for certain situations there are some in which its not:

  • Logging / debugging: Your normally seeing a message in a context so maintaining the prefix is unnecessary and not so 'pretty'
  • Encoding for clients: You may want to as in my example above render a 'pretty' version for your consuming clients, having the prefix again isn't so desirable: "type": "TypePayPal" vs "type": "PayPal"
  • Encoding for DB: As in my particular case I'd like to store a string instead of integers in my DB. Again having a column of Types all prefixed with the same 4 characters is just a waste of space given it would be in context in the column:
| id | type         |
|----|--------------|
| 0  | TypePayPal   |
| 1  | TypeUnknown  |
| 2  | TypeWorldPay |
| 3  | TypeAmazon   |

Proposal

Add an additional flag -trimprefix=<prefix> to x/tools/cmd/stringer, this will take a user defined prefix and strip it from the generated string, if no argument is passed it assumes the value of -type, heres an example with out a value for -trimprefix:

//go:generate stringer -type=Type -trimprefix
type Type uint8

const (
    TypeUnknown Type = iota
    TypePayPal
    TypeAmazon
    TypeWorldPay
)

Would generate:

fmt.Println(TypeUnknown.String())
// out: Unknown

fmt.Println(TypePayPal.String())
// out: PayPal

And a contrived example with a value -trimprefix=Ty:

//go:generate stringer -type=Type -trimprefix=Ty
type Type uint8

const (
    TypeUnknown Type = iota
    TypePayPal
    TypeAmazon
    TypeWorldPay
)

Would generate:

fmt.Println(TypeUnknown.String())
// out: peUnknown

fmt.Println(TypePayPal.String())
// out: pePayPal

I'd be happy to create a PR with the following feature if its accepted.

@quentinmit quentinmit added this to the Unreleased milestone Jul 29, 2016

@quentinmit

This comment has been minimized.

Show comment
Hide comment
@quentinmit

quentinmit Jul 29, 2016

Contributor

This seems reasonable to me, though I don't think you can assume a value of -trimprefix without breaking existing code. I think it has to be opt-in.

Contributor

quentinmit commented Jul 29, 2016

This seems reasonable to me, though I don't think you can assume a value of -trimprefix without breaking existing code. I think it has to be opt-in.

@adrianduke

This comment has been minimized.

Show comment
Hide comment
@adrianduke

adrianduke Jul 29, 2016

ok, I'll keep that in mind!

adrianduke commented Jul 29, 2016

ok, I'll keep that in mind!

@minux

This comment has been minimized.

Show comment
Hide comment
@minux

minux Jul 29, 2016

Member
Member

minux commented Jul 29, 2016

@jimmyfrasche

This comment has been minimized.

Show comment
Hide comment
@jimmyfrasche

jimmyfrasche Jul 29, 2016

Member

That would change the behavior of stringer, as @quentinmit pointed out.

Maybe a value that's not a valid identifier could trigger it without requiring spelling the prefix out? Something like -trimprefix - as a shorthand. If so, it should return an error if there's not a common prefix other than "" so the user notices the mistake.

Unrelated, but I assume that if you did

//go:generate stringer -type=Type -trimprefix=Type
type Type uint8

const (
    UnknownType Type = iota
    TypePayPal
    TypeAmazon
    TypeWorldPay
)

The strings would be

UnknownType
PayPal
Amazon
WorldPay

Is that correct?

Member

jimmyfrasche commented Jul 29, 2016

That would change the behavior of stringer, as @quentinmit pointed out.

Maybe a value that's not a valid identifier could trigger it without requiring spelling the prefix out? Something like -trimprefix - as a shorthand. If so, it should return an error if there's not a common prefix other than "" so the user notices the mistake.

Unrelated, but I assume that if you did

//go:generate stringer -type=Type -trimprefix=Type
type Type uint8

const (
    UnknownType Type = iota
    TypePayPal
    TypeAmazon
    TypeWorldPay
)

The strings would be

UnknownType
PayPal
Amazon
WorldPay

Is that correct?

@adrianduke

This comment has been minimized.

Show comment
Hide comment
@adrianduke

adrianduke Jul 29, 2016

@minux that was actually my first idea but I thought using the type name by default was simpler, I'm not opposed to going back to that idea though if thats the consensus.

@jimmyfrasche if we implemented @minux's idea of trimming the longest common prefix when you pass just a -trimprefix flag, would that resolve the issue of having to pass a signifying value like -? Or does that still change the behaviour? (I've yet to look at the stringer code). To your other question, yes I would expect if you passed a prefix to trim (Type) and a constant didn't start with that prefix (UnknownType), you just get the unchanged constant back (UnknownType), as per your example.

adrianduke commented Jul 29, 2016

@minux that was actually my first idea but I thought using the type name by default was simpler, I'm not opposed to going back to that idea though if thats the consensus.

@jimmyfrasche if we implemented @minux's idea of trimming the longest common prefix when you pass just a -trimprefix flag, would that resolve the issue of having to pass a signifying value like -? Or does that still change the behaviour? (I've yet to look at the stringer code). To your other question, yes I would expect if you passed a prefix to trim (Type) and a constant didn't start with that prefix (UnknownType), you just get the unchanged constant back (UnknownType), as per your example.

@jimmyfrasche

This comment has been minimized.

Show comment
Hide comment
@jimmyfrasche

jimmyfrasche Jul 29, 2016

Member

@adrianduke Assuming -trimprefix is a stdlib flag package flag.String

  • if you give it a default, that's what it is if the flag is not provided
  • if you call it just as -trimprefix without an argument, it'll error out saying you didn't provide anything to the -trimprefix flag
  • I'm not sure if you can create a custom flag that's optionally boolean and anyway that would be ambiguous.

You could have an -autotrimprefix and a -trimprefix but that seems messy. To me, having a magic name seems the best way to go, but it can't be something that either

  • a shell will interpret
  • the flag parser will interpret (in this case just = I think)
  • or is a valid Go identifier.

Of those left, - seemed the most mnemonic.

Member

jimmyfrasche commented Jul 29, 2016

@adrianduke Assuming -trimprefix is a stdlib flag package flag.String

  • if you give it a default, that's what it is if the flag is not provided
  • if you call it just as -trimprefix without an argument, it'll error out saying you didn't provide anything to the -trimprefix flag
  • I'm not sure if you can create a custom flag that's optionally boolean and anyway that would be ambiguous.

You could have an -autotrimprefix and a -trimprefix but that seems messy. To me, having a magic name seems the best way to go, but it can't be something that either

  • a shell will interpret
  • the flag parser will interpret (in this case just = I think)
  • or is a valid Go identifier.

Of those left, - seemed the most mnemonic.

@adrianduke

This comment has been minimized.

Show comment
Hide comment
@adrianduke

adrianduke Jul 30, 2016

@jimmyfrasche ah I understand now! There appears to be many valid special characters when placed as flag values (at least for BASH), heres a short list of some that make some amount of sense to me:

  • -trimprefix=-: would suggest to me some kind of placeholder with special meaning
  • -trimprefix=.: Current context in templates, suggests special meaning
  • -trimprefix=^: reminiscent of regex beginning of line character

Although having 2 separate flags doesn't seem that horrendous of an idea, having 1 flag with a special meaning character will most certainly be harder to get across to the user rather than your example of 2 flags:

  • -trimprefix=<prefix>: flag.String
  • -autotrimprefix: flag.Bool

flags are cheap and explicit, special characters are exceptions and harder to communicate.

adrianduke commented Jul 30, 2016

@jimmyfrasche ah I understand now! There appears to be many valid special characters when placed as flag values (at least for BASH), heres a short list of some that make some amount of sense to me:

  • -trimprefix=-: would suggest to me some kind of placeholder with special meaning
  • -trimprefix=.: Current context in templates, suggests special meaning
  • -trimprefix=^: reminiscent of regex beginning of line character

Although having 2 separate flags doesn't seem that horrendous of an idea, having 1 flag with a special meaning character will most certainly be harder to get across to the user rather than your example of 2 flags:

  • -trimprefix=<prefix>: flag.String
  • -autotrimprefix: flag.Bool

flags are cheap and explicit, special characters are exceptions and harder to communicate.

@adg adg removed the Proposal-Approved label Sep 12, 2016

@josharian josharian added the Suggested label Aug 7, 2017

@josharian josharian changed the title from proposal: x/tools/cmd/stringer Add a -trimprefix flag to x/tools/cmd/stringer: add a -trimprefix flag Aug 7, 2017

@mvdan

This comment has been minimized.

Show comment
Hide comment
@mvdan

mvdan Nov 8, 2017

Member

This would be useful for #15462, and I see the issue has been accepted for a while with little activity, so I have given it a go.

I have left out the automatic detection of prefixes for now. That can always be added later, with whatever extra flag or special trim value that we agree on.

Member

mvdan commented Nov 8, 2017

This would be useful for #15462, and I see the issue has been accepted for a while with little activity, so I have given it a go.

I have left out the automatic detection of prefixes for now. That can always be added later, with whatever extra flag or special trim value that we agree on.

@mvdan mvdan self-assigned this Nov 8, 2017

@mvdan

This comment has been minimized.

Show comment
Hide comment
Member

mvdan commented Nov 8, 2017

@adrianduke

This comment has been minimized.

Show comment
Hide comment
@adrianduke

adrianduke Nov 8, 2017

great job! This completely fell off my radar.

adrianduke commented Nov 8, 2017

great job! This completely fell off my radar.

@mvdan

This comment has been minimized.

Show comment
Hide comment
@mvdan

mvdan Nov 9, 2017

Member

This has been merged - not sure when gopherbot will catch up.

Member

mvdan commented Nov 9, 2017

This has been merged - not sure when gopherbot will catch up.

@mvdan

This comment has been minimized.

Show comment
Hide comment
@mvdan

mvdan Nov 9, 2017

Member

I have moved the discussion about automatic detection of prefixes to #22649.

Member

mvdan commented Nov 9, 2017

I have moved the discussion about automatic detection of prefixes to #22649.

@mvdan

This comment has been minimized.

Show comment
Hide comment
@mvdan

mvdan Nov 9, 2017

Member

Whoops, I forgot to use golang/go#N when closing this issue. Not gopherbot's fault at all.

Member

mvdan commented Nov 9, 2017

Whoops, I forgot to use golang/go#N when closing this issue. Not gopherbot's fault at all.

@mvdan mvdan closed this Nov 9, 2017

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Nov 10, 2017

Member

Whoops, I forgot to use golang/go#N when closing this issue.

@mvdan Are you using the latest version of git-codereview binary? I'm pretty sure it's supposed to catch that kind of mistake via pre-commit hooks or so. See here.

Member

dmitshur commented Nov 10, 2017

Whoops, I forgot to use golang/go#N when closing this issue.

@mvdan Are you using the latest version of git-codereview binary? I'm pretty sure it's supposed to catch that kind of mistake via pre-commit hooks or so. See here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment