-
-
Notifications
You must be signed in to change notification settings - Fork 22
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
Add more logging #76
Add more logging #76
Conversation
Signed-off-by: Geert Eltink <[email protected]>
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.
This is a great start, @geerteltink !
I'd also add logging in the various commands, before and/or after major activities (fetching/building changelogs, making API calls, etc.), as that will help us pinpoint where issues are occurring even if the services being called don't have logging integrated.
@@ -225,29 +228,29 @@ private function createMockChangelog(): string | |||
|
|||
private const CHANGELOG_STUB = <<< 'CHANGELOG' | |||
# Changelog | |||
|
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.
Do the tests that use these continue to work? I'd originally left these spaces in as they get trimmed when the nowdoc is evaluated by the engine, but I'm not sure what happens if they're omitted.
I wasn't sure about logging in the command only or the services. I guess both is better. |
When doing work through the API, it would also be great to output the response content when getting an error response from the API instead of only having the failed invariant saying "something went wrong" (or a similar generic message). |
That is an excellent idea: adding logging to the PSR-18 client should suffice, I suppose? |
Closing here: to be retried in a new PR, if needed, but the rebase work is equivalent to the dev work here :) |
Description
This PR ads more logging.