-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add thelper linter #1541
Add thelper linter #1541
Conversation
Hey, thank you for opening your first Pull Request ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Would you mind adding your linter and the available settings, even though default, to .golangci.example.yml
? This will be used to generate documentation at golangci-lint.run.
Could you also add tests for the rules t_first
and t_name
in test/testdata/thelper.go
?
@ldez, thank you for your suggestions. I've added config examples and tests for all available checks. |
I would like to suggest to you some changes to improve the configuration UX. I see 2 approaches:
I would prefer a boolean-based configuration because I don't have to know the name of the options. Slice based configuration # .golangci.example.yml
thelper:
checks:
- t_first
- t_name
- t_begin
- b_first
- b_name
- b_begin // pkg/config/config.go
type ThelperSettings struct {
Checks []string
} // pkg/golinters/thelper.go
cfgMap := map[string]map[string]interface{}{}
if cfg != nil {
cfgMap[a.Name] = map[string]interface{}{
"checks": strings.Join(cfg.Checks, ","),
}
} Boolean based configuration thelper:
test:
first: true
name: true
begin: true
benchmark:
first: true
name: true
begin: true type ThelperSettings struct {
Test struct {
First bool `mapstructure:"first"`
Name bool `mapstructure:"name"`
Begin bool `mapstructure:"begin"`
} `mapstructure:"test"`
Benchmark struct {
First bool `mapstructure:"first"`
Name bool `mapstructure:"name"`
Begin bool `mapstructure:"begin"`
} `mapstructure:"benchmark"`
} var opts []string
if cfg.Test.Name {
opts = append(opts, "t_name")
}
if cfg.Test.Begin {
opts = append(opts, "t_begin")
}
if cfg.Test.First {
opts = append(opts, "t_first")
}
if cfg.Benchmark.Name {
opts = append(opts, "b_name")
}
if cfg.Benchmark.Begin {
opts = append(opts, "b_begin")
}
if cfg.Benchmark.First {
opts = append(opts, "b_first")
}
cfgMap := map[string]map[string]interface{}{}
if cfg != nil {
cfgMap[a.Name] = map[string]interface{}{
"checks": strings.Join(opts, ","),
}
} WDYT? |
I think boolean-based configuration looks great 👍 I've copy-pasted it with a few modifications. |
have you thought about using tag/version? There are several advantages:
|
Well, I thought about thelper project in rolling release model. But I never work with dependabot before. Now I see versioning has sense for simple project too. PR is updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 👍
Hey, @kulti — we just merged your PR to
By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests. Thanks again! |
Hi, I'm the author of thelper linter.
Hope you find it useful and accept this PR.