Skip to content

Fix incorrect spacing handling in nolint directives #4561

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

Closed
796RCP92VZ opened this issue Mar 21, 2024 · 6 comments
Closed

Fix incorrect spacing handling in nolint directives #4561

796RCP92VZ opened this issue Mar 21, 2024 · 6 comments
Labels
area: nolint Related to nolint directive and nolintlint won't fix This will not be worked on

Comments

@796RCP92VZ
Copy link

796RCP92VZ commented Mar 21, 2024

No description provided.

@796RCP92VZ 796RCP92VZ added the bug Something isn't working label Mar 21, 2024
Copy link

boring-cyborg bot commented Mar 21, 2024

Hey, thank you for opening your first Issue ! 🙂 If you would like to contribute we have a guide for contributors.

@ldez ldez added won't fix This will not be worked on and removed bug Something isn't working labels Mar 21, 2024
@oschwald
Copy link

The "additionally" part mentioned in the issue hit someone on my team today. They originally wrote //nolint: revive and running golangci-lint with --fix turned it into // nolint: revive, which made nolintlint fail. I have a vague memory of nolintlint or some other linter preventing the space before the linter name in the past, but maybe I am misremembering.

@ldez ldez added the area: nolint Related to nolint directive and nolintlint label Mar 21, 2024
@ldez
Copy link
Member

ldez commented Mar 22, 2024

I put won't fix because:

  1. Your issue wants to lead to the same thing I already refused with your PR nolintlint: enforce directive and linter list formatting #4544.
  2. Some extra space cases are already fixed automatically, we have tests to prove that. (ex: // nolint:bob -> //nolint:bob)
  3. Your issue is a detail of a larger one (that we will not handle for now, intentionally, to avoid breaking changes)

The only thing we MAY do is create a nolintlint option to DETECT the invalid cases and create a report for them (a user should be able to enable/disable this behavior to avoid usage of exclude about them).

But this is not a "fix" for the incorrect spacing and this could not change the current behavior and the current supported syntax.

For me, your issue is off-topic by the goal it wants to achieve: "Fix incorrect spacing handling in nolint directives".

All those elements lead me to won't fix.

@ldez
Copy link
Member

ldez commented Mar 25, 2024

Sorry, but do you generate your answers with an AI?

You are writing a book with too many details each time, it's a bit discouraging to read.

If you are just a person who likes to write very long texts, don't take it badly, but I don't need an explanation about things that I already know, so you can go straight to the goal.

If you are using an AI, please generate a short answer.

I generated a summary of your comment with an AI:

The commenter acknowledges the larger issue at hand but argues for the value of implementing a specific improvement, suggesting a fix for a bug in the "nolint" tool.
They emphasize that this improvement, while minor, would enhance user experience and align with the goals of linters.
They express willingness to propose a design plan and implement the fix if deemed acceptable.
Additionally, they express appreciation for the community's efforts and offer to contribute positively.
The commenter also clarifies the goal of the fix and invites further discussion on the issue's scope and potential solutions.

@ldez
Copy link
Member

ldez commented Mar 25, 2024

It's been a bit frustrating that you respond immediately but ignore most of the content and dismiss what I'm saying

I think you have completely missed the meaning of my previous comment.

if you have time to respond immediately, you must surely have time to read more thoroughly and respond a little less promptly in exchange.

You are frustrated, I can understand but this sentence IS NOT ACCEPTABLE.

First warning.

@ldez
Copy link
Member

ldez commented Mar 25, 2024

We will not change nolint directives to follow strictly the convention.
nolint directives should be considered frozen with their imperfections.

I already said here the only acceptable possibility.

@ldez ldez closed this as completed Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: nolint Related to nolint directive and nolintlint won't fix This will not be worked on
Projects
None yet
Development

No branches or pull requests

3 participants