Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

core: use debug module for spawnResult error handling #4734

Merged
merged 1 commit into from
Feb 17, 2022

Conversation

lsqproduction
Copy link
Contributor

A quick change from console.log to debug module, as console logging the stderr was adding extra lines to the end of the migration output.

Copy link
Member

@cds-amal cds-amal left a comment

Choose a reason for hiding this comment

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

How can I test this?

@@ -272,7 +273,7 @@ class Console extends EventEmitter {
// Theoretically stderr can contain multiple errors.
// So let's just print it instead of throwing through
// the error handling mechanism. Bad call?
console.log(spawnResult.stderr.toString());
debug(spawnResult.stderr.toString());
Copy link
Member

Choose a reason for hiding this comment

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

spawnSync returns status, signal and error components in its return object. Do you think it's worth putting the debug behind a guard that checks iff there was an error? It might even be worth it to include those values when logging

Copy link
Contributor

Choose a reason for hiding this comment

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

We would get more info this way, right? If that's the case I don't think we should.

Copy link
Member

Choose a reason for hiding this comment

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

There could be more information, but it would only be presented if DEBUG environment is configured

@eggplantzzz
Copy link
Contributor

For reference, the PR that implemented this logging in the first place is #3794.

@cds-amal
Copy link
Member

For reference, the PR that implemented this logging in the first place is #3794.

welp, I forgot about that. 🤔

@cds-amal cds-amal self-requested a review February 16, 2022 16:17
Copy link
Member

@cds-amal cds-amal left a comment

Choose a reason for hiding this comment

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

@eggplantzzz convinced me it's ok to merge this PR, leaving the ora investigation as follow-on work.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants