Skip to content

Conversation

@denisvmedia
Copy link

Some revive rules have a more fine-grained way of setup. This PR improves the reference to uncover the full form of rule configuration.

@boring-cyborg
Copy link

boring-cyborg bot commented Feb 16, 2024

Hey, thank you for opening your first Pull Request !

@CLAassistant
Copy link

CLAassistant commented Feb 16, 2024

CLA assistant check
All committers have signed the CLA.

@ldez ldez self-requested a review February 16, 2024 13:27
@ldez
Copy link
Member

ldez commented Feb 19, 2024

I'm not sure to agree with the fact that documenting the fine-grained syntax is better than the simple one.
golangci-lint just provides an example, it's not exhaustive documentation of revive possibilities, and there is a link on each option to have more details.

@ldez ldez added the blocked Need's direct action from maintainer label Feb 19, 2024
@denisvmedia
Copy link
Author

I agree that this should taken from the original doc. However, my primary intent was to show that there can be another form of configuration. As it's probably not really evident that you can have keyed objects in golangci-lint config for some revive rules, when it gives arrays as an example.

@ldez
Copy link
Member

ldez commented Feb 19, 2024

As it's probably not really evident that you can have keyed objects in golangci-lint config for some revive rules, when it gives arrays as an example.

It's the same thing for the opposite.
There is no perfect solution because revive used some non-Go idiomatic configurations based on dynamic typing.

@denisvmedia
Copy link
Author

non-Go idiomatic configurations

I'm not sure that yaml configuration can be judged in go idioms. To me these two things are orthogonal.

My examples give full picture (with them it is possible to achieve the same as with the current reference example + more than that). So it's inclusive, but covers more.

@ldez
Copy link
Member

ldez commented Feb 19, 2024

I'm not sure that yaml configuration can be judged in go idioms.

revive is based on TOML-only configuration.

The structures related to the configuration are driven by the language, not the format.
Go is strongly typed so it's expected to use types and not any.

My examples give full picture (with them it is possible to achieve the same as with the current reference example + more than that). So it's inclusive, but covers more.

arguments can be a slice or a map, the both types are the full picture not just one.

I think the "simple" option is a better start than the "complex" one because if you are using the "complex" one there is 0 possibility that you need to use the "simple" one but the opposite is not true.

But as I already explained it's neither possible nor wanted to have an exhaustive revive options documentation.

@denisvmedia
Copy link
Author

revive is based on TOML-only configuration.

You are right. But here we use YAML.

The structures related to the configuration are driven by the language, not the format.

Honestly, for the user of the tool it doesn't matter in which language it is written (it so happened it's easier to write such a tool for go in go, but that's not a general rule for everything). So, it's more about the configuration flexibility rather than about the language. This is how I see it at least.

But as I already explained it's neither possible nor wanted to have an exhaustive revive options documentation.

I see your point. Then let's close the PR. At least it's good it will be historically here, it will be potentially discoverable.

@denisvmedia denisvmedia deleted the improve-revive-reference branch February 19, 2024 20:10
@ldez
Copy link
Member

ldez commented Feb 19, 2024

But here we use YAML.

golangci-lint supports TOML and YAML, but it's not important in this context.
I just wanted to explain the limitations and the problem of dynamic fields inside a configuration.

for the user of the tool it doesn't matter in which language it is written

It's exactly why for me, a configuration or an API should always follow strongly typed constraints: if you follow that, every language will be able to use it without extra custom parsing.

Otherwise, revive or golangci-lint are Go linters, so it's a bit expected to follow Go idioms.

@ldez
Copy link
Member

ldez commented Feb 19, 2024

The question behind that is "What do we want to provide as an example to new users?"

My philosophy is to always try to recommend the simple things because they mainly cover the majority of the use cases.

You may object to this that more explicit can be better, and in some contexts, I can agree with that.

It's a difficult task to arbitrate between simplicity (usage) and verbosity (understanding).

For me, it's like the "advanced mode option" inside some software, the parallel is not the best but I think you understand what I mean.

Today, I think the current doc is better but maybe in the future, my opinion can change, based on users' feedback or something else.

At least it's good it will be historically here, it will be potentially discoverable.

Your feedback is the first on this element, we will see what happens in the future.

In all cases, thank you for spending time to try to improve golangci-lint.

@ldez ldez added declined and removed blocked Need's direct action from maintainer labels Feb 19, 2024
@denisvmedia
Copy link
Author

denisvmedia commented Feb 19, 2024

Thank you for the clarifications. Anyway, I don't take it personally, and what you say sounds reasonable to me. I just happen to have a different opinion. We often have to make this or that choice and many times it is opinionated, and that's totally fine. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants