-
Notifications
You must be signed in to change notification settings - Fork 12
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
Improve logging and debugging #38
Improve logging and debugging #38
Conversation
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.
Love it! You can just leave the version update off, and we can get everything merged in and updated all at once. The rest looks great to me! Thanks so much for doing this
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.
Sweet!
Hey @russ / @jwoertink whenever you're ready, we can go ahead and start merging the other branches, except this PR. |
Thanks for the heads up @mjeffrey18. Right now @fernandes is on vacation, but once he gets back, we should be good to start merging stuff in. 👍 Thanks so much for tackling this, it's huge! |
…elpers Signed-off-by: Marc Jeffrey <mjeffrey18@gmail.com>
Signed-off-by: Marc Jeffrey <mjeffrey18@gmail.com>
Signed-off-by: Marc Jeffrey <mjeffrey18@gmail.com>
Signed-off-by: Marc Jeffrey <mjeffrey18@gmail.com>
Signed-off-by: Marc Jeffrey <mjeffrey18@gmail.com>
Signed-off-by: Marc Jeffrey <mjeffrey18@gmail.com>
Signed-off-by: Marc Jeffrey <mjeffrey18@gmail.com>
52fe0ec
to
39d5cc3
Compare
@fernandes this one is ready for final review/merge Big shout out again to @jgaskins as a lot of the inspiration for the cleanup came from his fork 💪 |
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 so much for this!
@mjeffrey18 what's the rational behind removing these messages? I'm asking because it helped me in the past to catch a few bugs lol
maybe my first comment answers this one on description 😉 if this is just because now we are using a bare crystal log class, it supports capturing the logs for testing, like I did here |
Hey @fernandes, your right, logging can absolutely catch bugs. I've always found solid specs are way better at catching bugs and logs are better for debugging/monitoring. Taking some lessons I've learned from heavy use of Rspec over the years;
However, I'm open to other alternative approaches (as your suggested) if we feel testing the logs is worth it for this particular case. |
I agree, I tried to test the behavior as much as possible
thank you! 🙇 my final thought is if we should handle log messages as part as the software behavior/functionality, because if so, it's good to have it tested, once users will get the correct messages in the tested scenario comments? 😄 |
Fixes: #19
Let me know what you guys think?