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

golines breaks lines with //nolint #93

Open
guettli opened this issue Jan 27, 2023 · 3 comments
Open

golines breaks lines with //nolint #93

guettli opened this issue Jan 27, 2023 · 3 comments

Comments

@guettli
Copy link

guettli commented Jan 27, 2023

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
Copy link
Contributor

telemachus commented Jan 27, 2023

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
Copy link
Author

guettli commented Jan 30, 2023

@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
Copy link
Contributor

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants