-
Notifications
You must be signed in to change notification settings - Fork 21
gbn+mailbox: cleanup & prefixed logger #90
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
Conversation
0daa526 to
47b022c
Compare
ViktorT-11
left a comment
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 a lot for this!! This will be really, really helpful 🙏:fire:!!
Left a few comments below, but also leaving one extra feedback here:
Should we change the mailbox/server_conn.go & mailbox/client_conn.go to also use a prefixed logger? Those structs currently have quite a few log calls that adds a separate "Server" & "Client" prefix.
Also, should I change my packeting bug PRs to be based on this already?
5fa6e72 to
ef7b4fc
Compare
ellemouton
left a comment
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 @ViktorTigerstrom ! Added the prefixed logger for the mailbox package as you suggested 🙏
Yeah I think that would be a good idea just so that you get the advantages of the prefixed logger & so that you can take advantage of the slightly cleaner GBN structure. You can change the base of you PR to |
ViktorT-11
left a comment
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.
tACK LGTM, thanks for this 🙏🔥!!
guggero
left a comment
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.
Very nice! Just a couple of nits, otherwise LGTM 🎉
Instead of manually needing to insert "isServer=" everywhere.
ef7b4fc to
65256cb
Compare
65256cb to
72e5adf
Compare
ellemouton
left a comment
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 Y'all 🤓
This PR introduces a prefixed logger which is then passed to the client and server so that
their logs are prefixed with
(client)and(server)so that we dont have to manually usethe
isServervariable in logs.Then, some variables are moved out of the GBN struct and into a new
configstruct.depends on #92