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

feat: allow add custom logger #19

Merged
merged 19 commits into from
Aug 29, 2022
Merged

Conversation

ttys3
Copy link
Collaborator

@ttys3 ttys3 commented Aug 26, 2022

feat: allow add custom logger, also allow others like golangci-lint to have custom logger checker configs

we wrap zap logger in most cases, so I hope this linter can also allow user to check zap, logr or klog based wrapper or anyother logger.

update:

refactored to using a config file to define custom loggers.

the config file format is yaml

sample config file:

# loggercheck sample config
disable:
    - klog
    - logr
    - zap
custom-checkers:
    - name: customlogger
      package-import: example.com/customlogger
      funcs:
        - example.com/customlogger.Debug
        - example.com/customlogger.Info
        - example.com/customlogger.Warn
        - example.com/customlogger.Error
    - name: mylogger
      package-import: foo.example.com/bar
      funcs:
        - (*foo.example.com/bar.Logger).Debugw
        - (*foo.example.com/bar.Logger).Infow
        - (*foo.example.com/bar.Logger).Warnw
        - (*foo.example.com/bar.Logger).Errorw

@codecov
Copy link

codecov bot commented Aug 26, 2022

Codecov Report

Merging #19 (f799a59) into master (a05b1e1) will increase coverage by 3.00%.
The diff coverage is 86.93%.

❗ Current head f799a59 differs from pull request most recent head 20319dc. Consider uploading reports for the commit 20319dc to get more accurate results

@@            Coverage Diff             @@
##           master      #19      +/-   ##
==========================================
+ Coverage   82.14%   85.14%   +3.00%     
==========================================
  Files           6       10       +4     
  Lines         168      303     +135     
==========================================
+ Hits          138      258     +120     
- Misses         22       31       +9     
- Partials        8       14       +6     
Impacted Files Coverage Δ
checker.go 45.45% <45.45%> (-54.55%) ⬇️
flags.go 81.57% <80.95%> (-0.24%) ⬇️
loggercheck.go 85.55% <87.50%> (+3.90%) ⬆️
rules/rules.go 88.88% <88.88%> (ø)
config.go 100.00% <100.00%> (ø)
internal/bytebufferpool/pool.go 100.00% <100.00%> (ø)
options.go 100.00% <100.00%> (ø)
sets/stringset.go 100.00% <100.00%> (ø)

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

@timonwong
Copy link
Owner

@ttys3 Thanks, but for now I'll hold this pr for a while.

Because I decide to rename this repo, since it's not logr specific anymore :)

loggercheck.go Outdated Show resolved Hide resolved
@ttys3 ttys3 changed the title feat: allow add custom logger WIP: feat: allow add custom logger Aug 26, 2022
loggercheck.go Outdated Show resolved Hide resolved
@ttys3 ttys3 changed the title WIP: feat: allow add custom logger feat: allow add custom logger Aug 26, 2022
@timonwong
Copy link
Owner

I have some ideas, I'll apply some changes to your PR

@ttys3
Copy link
Collaborator Author

ttys3 commented Aug 27, 2022

OK.

the main idea purpose is:

  1. allow tools like golanci-lint can config custom logger and other opion (currently with WithConfig Option)
  2. and this also works with the cmd cli flags (currently via the -config)

@timonwong
Copy link
Owner

@ttys3 I've pushed my changes, PTAL

Signed-off-by: Timon Wong <[email protected]>
@timonwong
Copy link
Owner

I should rename pattern to rules

@timonwong timonwong merged commit 2662777 into timonwong:master Aug 29, 2022
@timonwong
Copy link
Owner

Thanks!

@ttys3
Copy link
Collaborator Author

ttys3 commented Aug 29, 2022

I have some idea on the exported Config struct.
I think it's better keep the consistency between cli and golabgci-lint user

@ttys3
Copy link
Collaborator Author

ttys3 commented Aug 29, 2022

I'm try to make a PR to show the idea when I have time

@timonwong
Copy link
Owner

timonwong commented Aug 29, 2022

@ttys3 I think the clean way to do that is re-implement singlechecker.Main. So we accept string list for "disable" and "rules" and do the parse later prior to run.

Or we can doing the parse during "run", though not very clean.

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.

2 participants