segmentio / golines

A golang formatter that fixes long lines
MIT License
903 stars 56 forks source link

golines breaks lines with //nolint #93

Open guettli opened 1 year ago

guettli commented 1 year ago

Original line:

    deviceID, err := strconv.ParseInt(strings.TrimPrefix(node.Spec.ProviderID, providerPrefix), 10, 32) //nolint:revive // https://www.reddit.com/r/golang/comments/10mi0el/strconvparseintmyid_10_32_avoid_magic_numbers/

after running golines:

    deviceID, err := strconv.ParseInt(
        strings.TrimPrefix(node.Spec.ProviderID, providerPrefix),
        10,
        32,
    ) //nolint:revive // https://www.reddit.com/r/golang/comments/10mi0el/strconvparseintmyid_10_32_avoid_magic_numbers/

I like the output of golines.

But now I get warnings from the linter:

foo/bar.go:55:3: add-constant: avoid magic numbers like '10', create a named constant for it (revive) 10, ^ foo/bar.go:56:3: add-constant: avoid magic numbers like '32', create a named constant for it (revive) 32,

I think it would be great if golines would skip lines containing "//nolint" by default.

What do you think?

telemachus commented 1 year ago

I can see an argument for skipping lines that contain //nolint special comments. That is, I can imagine cases where just a //nolint comment causes an undesired break. I'm not sure whether I think the change is justified or not, but it's worth considering.

But in this case, I think there's a much simpler fix than patching golines: you can simply remove the (very long) second comment with the Reddit URL. That would probably leave you with a line short enough that golines would ignore it.

guettli commented 1 year ago

@telemachus is there already a way to configure golines, so that lines containing a special string get ignored?

If not, would you accept a patch?

telemachus commented 1 year ago

@telemachus is there already a way to configure golines, so that lines containing a special string get ignored?

I don't think so.

If not, would you accept a patch?

I'm not the maintainer, but I can say that a similar request is labelled as an enhancement. That suggests to me that a patch would at least be considered, but I can't be sure.