-
Notifications
You must be signed in to change notification settings - Fork 84
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: client debug logging #58
base: main
Are you sure you want to change the base?
Conversation
Logging is necessary to know what is going on in the program. This commit creates some logging statements here and there. The following commits will be better scoped.
Trace is a more verbose logging level used to follow the flow of the program. However, it has not been implemented yet.
What is the status of this PR ? Do you want me to drop a review anytime soon ? |
Hi! Sorry for the lack of communication. I have been kept busy with exam preparations. In my opinion, the request is ready to be reviewed (I'll do my best to keep up with resoling conflicts and change requests). I can't seem to request reviews, though… Could you confirm or correct the proposed debugging level convention, from above? I'd really help me to make sure that I use the appropriate level, as (atm) everything is debug and I think I went a bit too much into detail. |
Allright, focus on your exams, I'll do the rebasing and make a review |
Okay so I did not see that the branches still differ quite a lot. We can easily wait until the end of your exams and then look into it into more details. The changes you added are valuable, but the ssh3 upstream evolved quite a lot since its initial release (+ files have been renamed), so we should probably adapt these changes to the current state once you have time. :-) |
Apologies for not keeping up with the main branch. I managed to merge the main branch. Checking the changes over in the files changed tab shows that there are only additions. After skimming over them, I am confident that I did not create side effects. 😄 I hope everything is ready to be reviewed — let me know otherwise. |
Hi @francoismichel ! I'm all done with exams now and am available again to contribute to the project. I just merged it with the latest reference from the main branch, so it should be up-to-date. Could you review this PR? |
Hello everyone!
tl;dr: this PR improves logging.
While debugging #35 and #38, I have added some extra logging statements here and there in the application to be able to follow the execution flow. I then set out to add logging a bit everywhere.
My logging rationale is as follows
trace
: used to trace the code, follow execution flow and find one exact code part.debug
: information and output that is diagnostically helpful to understand what is going on.info
: general use informationwarn
: information about oddities, but for which recovering is planned.error
: operation results that are expected to work but did not, but do not force program termination.fatal
: circumstances that force the program to terminate.Does that sound right? At the moment, I have been using mostly
debug
, as there is no option fortrace
. Should that be implemented?Also, is it okay to give warnings (and up) to the user, or should there be a separate feedback for the user (on
stderr
)?On the subject of the code, I have just added logging statements in my style where I think them useful. Am I too aggressive? Is my (variable) logging convention acceptable for the project?
Overall, please share what you think about the changes and what else I should incorporate ^^.