Skip to content

Fixed the input and output utilities#808

Merged
leo merged 2 commits into
masterfrom
fix-teams-and-cc
Sep 5, 2017
Merged

Fixed the input and output utilities#808
leo merged 2 commits into
masterfrom
fix-teams-and-cc

Conversation

@leo

@leo leo commented Sep 5, 2017

Copy link
Copy Markdown
Contributor

After this PR, the input fields will work properly again and no errors will be swallowed.

@leo leo requested a review from Qix- September 5, 2017 09:39
@Qix-

Qix- commented Sep 5, 2017

Copy link
Copy Markdown
Contributor

Any reason why error() doesn't just output the message? Wrapping all occurrences of error() with console.error() isn't very DRY.

@leo

leo commented Sep 5, 2017

Copy link
Copy Markdown
Contributor Author

@Qix- @rauchg said it's very important that a function only does as less as possible (like composing a string) - he criticized that on Now Desktop as well, so I converted it into log statements that are logging a string that was composed by a function.

The statements you in the code here were written by him before, so I just extended that.

BTW: I did it just like you described in all of my code before he told me to do it otherwise.

@Qix-

Qix- commented Sep 5, 2017

Copy link
Copy Markdown
Contributor

I just worry that someone is going to mistake error() for actually outputting an error message, thus running the risk of an error message silently being ignored.

@Qix- Qix- left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, glad to see this fixed. :)

@leo leo merged commit f67af44 into master Sep 5, 2017
@leo leo deleted the fix-teams-and-cc branch September 5, 2017 10:03
@matheuss

matheuss commented Sep 5, 2017

Copy link
Copy Markdown
Contributor

@Qix- @leo we moved away from having output utilities to console.log because having side effects like that makes it very hard (if not impossible) to perform nested calls like this:

console.log(info(
  `AWS credentials found in ${param(AWS_CREDENTIALS_FILE_PATH)}.`,
  `  Would you like to use them?`
))

@Qix-

Qix- commented Sep 5, 2017

Copy link
Copy Markdown
Contributor

@matheuss but in that case, console.log wouldn't be necessary? I'm not following that specific logic.

I agree with the sentiment, but I don't necessarily like the non-DRY-ness of it all - especially since, as I mentioned before, it'd be easy for someone to forget a logging statement and accidentally hide an error message because they didn't know to output to the console.

We see this kind of problem frequently over at Chalk (where people don't realize they need to console.log() the result of chalk calls).

I just worry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants