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

refactor: make config options be consistent with the cli style #22

Merged
merged 8 commits into from Aug 29, 2022

Conversation

ttys3
Copy link
Collaborator

@ttys3 ttys3 commented Aug 29, 2022

simplify Config, make it consistent with the cli style

  1. when users using the cli tool could just using a simple rule files,
    there's no reason to make users in other tools config the linter in a harder way.

  2. map[string]struct{} is not a good struct for config file like json, yaml etc.

for example in yaml people will force to config like this:

- a: {}
- b: {}
- c: {}

but the whole thing they wanted is just:

- a
- b
- c

@codecov
Copy link

codecov bot commented Aug 29, 2022

Codecov Report

Merging #22 (f5e5cd7) into master (ec4e3b8) will decrease coverage by 0.49%.
The diff coverage is 87.50%.

❗ Current head f5e5cd7 differs from pull request most recent head 2f4a6c6. Consider uploading reports for the commit 2f4a6c6 to get more accurate results

@@            Coverage Diff             @@
##           master      #22      +/-   ##
==========================================
- Coverage   91.08%   90.59%   -0.50%     
==========================================
  Files          10        6       -4     
  Lines         303      287      -16     
==========================================
- Hits          276      260      -16     
+ Misses         19       17       -2     
- Partials        8       10       +2     
Impacted Files Coverage Δ
internal/rules/rules.go 95.49% <ø> (ø)
staticrules.go 45.45% <ø> (ø)
loggercheck.go 86.08% <79.31%> (-2.81%) ⬇️
internal/sets/string.go 100.00% <100.00%> (ø)
options.go 100.00% <100.00%> (ø)
cmd/loggercheck/main.go
plugin/plugin.go

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@timonwong
Copy link
Owner

2. map[string]struct{} is not a good struct for config file like json, yaml etc.

The config is not intended to used as config file.

@timonwong
Copy link
Owner

timonwong commented Aug 29, 2022

Though simplify Config struct for easier integration is good.

For golangci-lint, they have their own config struct styles, so there must be a struct conversion in between:
golangci/golangci-lint#3144

@ttys3
Copy link
Collaborator Author

ttys3 commented Aug 29, 2022

even not intended to used as config file, even it is not for end user.

when the thing can be []string, why would we let people construct a map[string]struct{} ?

I know golangci-lint has its own config file or struct.

I mean, for Developer Experience (DX)

map[string]struct{} is an internal struct we use in loggercheck.

something like []string is we expose to the user (a developer or an end user)

@timonwong
Copy link
Owner

even not intended to used as config file, even it is not for end user.

when the thing can be []string, why would we let people construct a map[string]struct{} ?

I know golangci-lint has its own config file or struct.

I mean, for Developer Experience (DX)

Yes, it's good to use string slice over sets

@ttys3
Copy link
Collaborator Author

ttys3 commented Aug 29, 2022

there's another thing, since we parse the rules our self, so there's failure.
I think we should change func NewAnalyzer(opts ...Option) *analysis.Analyzer to

func NewAnalyzer(opts ...Option) (*analysis.Analyzer, error)

then we can let the caller to determinate whether to panic or print an error message

@timonwong
Copy link
Owner

there's another thing, since we parse the rules our self, so there's failure. I think we should change func NewAnalyzer(opts ...Option) *analysis.Analyzer to

func NewAnalyzer(opts ...Option) (*analysis.Analyzer, error)

then we can let the caller to determinate whether to panic or print an error message

I think we can move those parts in run(). So we can process flags & config in a more consistent way. (Since we cannot add hooks for the global fags)

@ttys3
Copy link
Collaborator Author

ttys3 commented Aug 29, 2022

and also, loggercheck.WithConfig could split to:

loggercheck.WithDisable and loggercheck.WithRules,

we do not need to expose an Config struct for developers since ourselves actually not using that

@ttys3
Copy link
Collaborator Author

ttys3 commented Aug 29, 2022

it is better not move those to run()

because run() will make those run multi times and event concurrently (if I'm not wrong with this)

thus, we will need introduce sync.Once

I do not think this is a good idea

keep it in the NewXXXX construct is reaonable

@timonwong
Copy link
Owner

timonwong commented Aug 29, 2022

because run() will make those run multi times and event concurrently (if I'm not wrong with this)

it will b called only once (guarded in a sync.Once)

@ttys3
Copy link
Collaborator Author

ttys3 commented Aug 29, 2022

I made a second commit, PTAL @timonwong

274a646

Signed-off-by: Timon Wong <timon86.wang@gmail.com>
Signed-off-by: Timon Wong <timon86.wang@gmail.com>
Signed-off-by: Timon Wong <timon86.wang@gmail.com>
Signed-off-by: Timon Wong <timon86.wang@gmail.com>
Signed-off-by: Timon Wong <timon86.wang@gmail.com>
@timonwong timonwong merged commit ac84b09 into timonwong:master Aug 29, 2022
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

Successfully merging this pull request may close these issues.

None yet

2 participants