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

🌱 Extract logger package #450

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

abrugaro
Copy link
Contributor

No description provided.

Signed-off-by: Alejandro Brugarolas <[email protected]>
var log *logr.Logger
var logrusLog *logrus.Logger

func InitLogger() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need the ability to use the log level to increase verbosity.

I am also curious why instead of logs on the objects, we are miving to global functions, not opposed, just wondering the reasoning.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is part of the work related to #249. Currently, the AnalyzeCommand struct is pretty big, and it's being passed around to most of the project's functions just because they need access to logging

In fact, I haven't yet moved the providers to a separate package precisely because they depend on the logger currently located within the AnalyzeCommand so its a circular dependency.

Another approach I can think of to separate the logger from AnalyzeCommand would be to modify each function to include an extra parameter for the logger.

And I saw that this was reccommended: https://pkg.go.dev/github.com/sirupsen/[email protected]#New

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A change to remove passing loggers around would impact alot of stuff and currently most analyzer-lsp structs take a logger.

Can you show some functions that need the logger?

Copy link
Contributor

@jmle jmle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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.

3 participants