dmarkham / enumer

A Go tool to auto generate methods for your enums
Other
411 stars 63 forks source link

Transformer and other operations when 'linecomment' #34

Closed xescugc closed 4 years ago

xescugc commented 4 years ago

When using the -linecomment=true I think that operations that change the text should not apply, for example -transform= -trimprefix= -addprefix= as you are saying how do you want it to look like:

  -linecomment
        use line comment text as printed text when present

We had an error due to that (but was our fault) as we had a type like:

package enumer

type DeltaType int

//go:generate enumer -type=DeltaType -transform=snake -output=delta_type_string.go -linecomment=true
const (
    Common    DeltaType = iota + 1 // =
    LeftOnly                       // -
    RightOnly                      // +
    Unknown                        // ?
)

It was a copy paste error but it ended up having a .String() that always returned "" as the linecomment was not "valid" to snake (which could potentially rise errors if a type is gonna be transformed to just ""? :thinking: ).

If we want to change this behavior I'm open to do a PR which will also solve https://github.com/dmarkham/enumer/issues/27 :smile:

dmarkham commented 4 years ago

Very interesting . I have mixed feeling on this. I'm just trying to think of a case where someone would want to use both, it would be strange.

dmarkham commented 4 years ago

So I think this will be a good idea I would like to not fail silently. So maybe if you use both options enumer errors out. @xescugc thoughts?

xescugc commented 4 years ago

To be sure, (-transform= || -trimprefix= || -addprefix=) && -linecomment is invalid.

I've look over all the codebase and try to find one use-case in which I would need a combination of the -linecomment and any of the "transformers" and I found one! And it's OSS so I can directly share the link https://github.com/cycloidio/terracognita/blob/aed731da28e613b69625fad7c0a77e5c7edee996/aws/resources.go#L77. So in this case the previous assumption would not work (it transforms it to iamsaml_provider without the linecomment).

But either way I would not add the limitation to use a combination (as in that case we also use the transform and addprefix hehe).

The main problem for me was to have an "" generated and I would not mind to have or not the trimprefix and addprefix applied to the linecomment as it would make sense in general (as if you add it to all also add it to that one) but I would not mind not having it as IMO the linecomment is more of an "edge case" so if you have to manually add/remove a prefix it's not a big deal (but I could be wrong with that). For example on the code I linked, I would not mind adding aws_ on the linecomment one.

So I can think on different solutions: 1) Allow everything but a if a "" is gonna be generated FAIL 1) Same as last one, but only FAIL if before the transformer it had a value 1) Do not apply -transformer to the -linecomment parts 1) Same as last one but -linecomment is the last value so no-transform=, -trimprefix=, -addprefix= is applied

I think the most conservative is the 2 as it keeps the same behavior we have right now and it solves the "root" issue.

So if we agree I'll also change the documentation to mention the order of the changes:

So people know the order and also that the -linecomment it's not the last but the first value.

Tel me WDYT and if it's ok I'll open a PR :smile:

dmarkham commented 4 years ago

@xescugc I also really like 2. as you have it listed above. I would love to see a patch that cleans this up, thank you for your help.