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

Logging refactoring #350

Merged
merged 6 commits into from
Apr 5, 2024

Conversation

MatrixCrawler
Copy link
Collaborator

This is an attempt to use a logger for output information and consolidate the different outputs.

@MatrixCrawler
Copy link
Collaborator Author

MatrixCrawler commented Apr 2, 2024

Here are some example outputs

image
image

What do you think regarding that new output @warrensbox

lib/logging.go Outdated Show resolved Hide resolved
lib/logging.go Outdated Show resolved Hide resolved
lib/logging.go Outdated Show resolved Hide resolved
@yermulnik
Copy link
Collaborator

What do you think regarding that new output @warrensbox

I like it. Especially if add colors to messages. It also standardizes logging across the whole app which great.

@MatrixCrawler MatrixCrawler added the enhancement Refactor existing code for better performance and quality label Apr 2, 2024
@MatrixCrawler MatrixCrawler marked this pull request as ready for review April 2, 2024 13:40
lib/logging.go Outdated Show resolved Hide resolved
@warrensbox
Copy link
Owner

@MatrixCrawler please go ahead and add the color output. Otherwise, all look good

@MatrixCrawler MatrixCrawler force-pushed the logging-refactoring branch 2 times, most recently from 8a85298 to 0d07397 Compare April 3, 2024 07:11
@MatrixCrawler
Copy link
Collaborator Author

@MatrixCrawler please go ahead and add the color output. Otherwise, all look good

I did add the color to the output configuration.

@MatrixCrawler MatrixCrawler self-assigned this Apr 3, 2024
go.mod Outdated Show resolved Hide resolved
@MatrixCrawler
Copy link
Collaborator Author

MatrixCrawler commented Apr 3, 2024

Here are the new screenshots using the Charmbracelet logger

image
image

This is the example with the Gookit logger
image
image

Personally i favour the Gookit logger a bit more.

@yermulnik
Copy link
Collaborator

yermulnik commented Apr 3, 2024

Here are the new screenshots using the Charmbracelet logger

Looks nice.
Is it possible to remove those extra newlines that produce extra empty lines between logger messages?

@MatrixCrawler
Copy link
Collaborator Author

MatrixCrawler commented Apr 3, 2024

Here are the new screenshots using the Charmbracelet logger

Looks nice. Is it possible to remove those extra newlines that produce extra empty lines between logger messages?

We will.. there are \n on some log messages. Will remove them

@yermulnik
Copy link
Collaborator

Also any idea what this is in Charmbacelet logger output? 🤔
image

@yermulnik
Copy link
Collaborator

Looks nice. Is it possible to remove those extra newlines that produce extra empty lines between logger messages?

We will.. there are \n on some log messages. Will remove them

Just wanted to post the same about newlines =)

@MatrixCrawler
Copy link
Collaborator Author

Removed the newlines
image

@yermulnik
Copy link
Collaborator

Personally i favour the Gookit logger a bit more.

I'm fine with both =)
From the Gookit logger I'd drop the [application] bit though (and maybe square brackets around timestamtp and log level).

@MatrixCrawler
Copy link
Collaborator Author

Personally i favour the Gookit logger a bit more.

I'm fine with both =) From the Gookit logger I'd drop the [application] bit though (and maybe square brackets around timestamtp and log level).

How's this?
image

@yermulnik
Copy link
Collaborator

I'm fine with both =) From the Gookit logger I'd drop the [application] bit though (and maybe square brackets around timestamtp and log level).

How's this?

Amazing ❤️

@MatrixCrawler
Copy link
Collaborator Author

Then this is ready for next review round

@yermulnik
Copy link
Collaborator

yermulnik commented Apr 3, 2024

Then this is ready for next review round

image

@MatrixCrawler MatrixCrawler force-pushed the logging-refactoring branch 2 times, most recently from e445483 to f695ab9 Compare April 3, 2024 18:34
@MatrixCrawler
Copy link
Collaborator Author

MatrixCrawler commented Apr 3, 2024

Then this is ready for next review round

image

Rebased and Squashed commits

Copy link
Collaborator

@yermulnik yermulnik left a comment

Choose a reason for hiding this comment

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

I attempted to align all logger messages between each other. Hope you don't mind.
Please feel free to adjust where and if necessary.

lib/common_test.go Outdated Show resolved Hide resolved
lib/dir_perm_windows.go Outdated Show resolved Hide resolved
lib/dir_perm_windows.go Outdated Show resolved Hide resolved
lib/dir_perm_windows.go Outdated Show resolved Hide resolved
lib/common_test.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@MatrixCrawler
Copy link
Collaborator Author

I attempted to align all logger messages between each other. Hope you don't mind. Please feel free to adjust where and if necessary.

Thanks. Much appreciated 😁

Copy link
Collaborator

@yermulnik yermulnik left a comment

Choose a reason for hiding this comment

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

@warrensbox Are you ok with this change?

@yermulnik
Copy link
Collaborator

@MatrixCrawler Could you please resolve merge conflict and we should be good to merge.

@MatrixCrawler MatrixCrawler merged commit fa36c17 into warrensbox:master Apr 5, 2024
@MatrixCrawler MatrixCrawler deleted the logging-refactoring branch April 5, 2024 09:26
@MatrixCrawler MatrixCrawler added this to the Release 1.1.0 milestone Apr 5, 2024
@MatrixCrawler MatrixCrawler added this to the Release 1.1.0 milestone Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Refactor existing code for better performance and quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants