formatting can change struct tags inside the ticks

[tommyjcarpenter]

The following code

type QuotaList struct {
	metav1.TypeMeta `json:",inline"`
	metav1.ListMeta `json:"metadata,omitempty"`
	Items           []Quota `json:"items"`
}

will change to

type QuotaList struct {
	metav1.TypeMeta `        json:",inline"`
	metav1.ListMeta `        json:"metadata,omitempty"`
	Items           []Quota `json:"items"`
}

easier to see in this screenshot:

is this correct behavior?

[ldez]

This happens because --reformat-tags is true by default, and inside the file, you have a struct with multiple tags.

before

package main

type TypeMeta struct{}

type ListMeta struct{}

type Quota struct{}

type QuotaList struct {
	TypeMeta `json:",inline"`
	ListMeta `json:"metadata,omitempty"`
	Items    []Quota `json:"items"`
}

type Foo struct {
	Value string `json:"value" yaml:"value"` // <- multiple tags
	Name  string `yaml:"name" json:"name" ` // <- multiple tags
}

after

package main

type TypeMeta struct{}

type ListMeta struct{}

type Quota struct{}

type QuotaList struct {
	TypeMeta `        json:",inline"`
	ListMeta `        json:"metadata,omitempty"`
	Items    []Quota `json:"items"`
}

type Foo struct {
	Value string `json:"value" yaml:"value"` // <- multiple tags
	Name  string `json:"name"  yaml:"name"` // <- multiple tags
}

The option --reformat-tags reorders and "aligns" the tags.

If you want to disable the behavior, use --no-reformat-tags.

Note: I'm not related to Segment, I'm not a maintainer of segmentio/golines.

[tommyjcarpenter]

@ldez Sorry; that is not what I meant. I realize why this feature is on.

The question is whether this feature is correct, given that it actually changes the data inside the tag, vs

Compare

type QuotaList struct {
	metav1.TypeMeta `        json:",inline"`
	metav1.ListMeta `        json:"metadata,omitempty"`
	Items           []Quota `json:"items"`
}

to

type QuotaList struct {
	metav1.TypeMeta         `json:",inline"`
	metav1.ListMeta         `json:"metadata,omitempty"`
	Items           []Quota `json:"items"`
}

the former actually changes the compiled binary; some people hold the opinion that formatters should not result in an actual binary change.

[ldez]

golines is compatible with go fmt, because a formatter must be compatible with go fmt.

The indentation before the backtick is not compatible with go fmt, so it's impossible to do it.

That's why the indentation is inside the structtag.

[kanaka]

Adding alignment within tags was rejected by go fmt as potentially having unintended side-effects: golang/go#16918

The original request for this feature is here: #1. I suspect the go fmt rejection of this functionality was not considered when this was implemented. Perhaps it shouldn't enabled by default?

[ldez]

I'm working on a fork.

My opinion on this part is here.

I'm waiting for Segment feedback before creating a tag and introducing breaking changes.

[tommyjcarpenter]

I think the "reasoning" is different between these two; actually changing potentially semantic data vs "its making lines longer vs shorter"

[ldez]

But in the end, this is the same result: the reformat-tags option should not be true by default, and the "align" should be either removed or optional.

By its very nature, the reformat-tags option is potentially changing semantics even without the "align", because it reorders structtags.
A structtag is just a string, so any changes inside this string will result in modifications of the compiled binary.

And as I already explained, indentation before backticks is not possible.

[tommyjcarpenter]

Yes, I understand that the "preferred" alignment is not possible. In this case my opinion would probably be to scrap this feature, or as you said atleast make it not the default, and in the docs for the feature, reference the "potentially harmful" link that @kanaka linked.

[ldez]

For me, this option is off-topic and has some important flaws (the semantics modification is just one of them).

I don't want to break before the first tag on the fork, as explained here, because I think users will prefer a seamless migration.
But I will "deprecate" the option inside the documentation because I already know the current option will be removed (and maybe replaced).

golangci#12