Skip to content
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

console.log support for multiple arguments #433

Merged
merged 5 commits into from
Jan 24, 2018
Merged

Conversation

qwIvan
Copy link
Contributor

@qwIvan qwIvan commented Jan 17, 2018

such as console.log('Hello', 'World', '!');
image

@welcome
Copy link

welcome bot commented Jan 17, 2018

💖 Thanks for opening this pull request! 💖
Here is a list of things that will help get it across the finish line: - If this is a new or updated CSS interactive example, please ensure that you followed the CSS styleguide - If this is a new or updated JavaScript interactive example, please ensure that you followed the JavaScript styleguide - If your changes affects any of the steps in our contribution docs, please also make the relevant changes there.

'use strict';
var output = '';
output = output + this.formatOutput(args[0]);
for (var i=1; i<args.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Code style nits: Change to

for (var i = 1, l = args.length; i < l; i++) {

console.log = function(loggedItem) {
consoleUtils.writeOutput(consoleUtils.formatOutput(loggedItem));
console.log = function() {
consoleUtils.writeOutput(consoleUtils.concatOutput(arguments));
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps the context is making this a little blurry but, it seems that this change will mean we loose the formatting done by formatOutput. That is definitely something we do not want to happen. Perhaps we should instead loop over the arguments, passing each one to the format function before concatenating together?

@schalkneethling
Copy link
Contributor

@qwIvan Thank you for your contribution. Couple of items to review. I also want to note that, unless you change your current avatar, I will not in good faith be able to merge this pull request and add you as a contributor.

The image depicts something that I feel is disrespectful to woman, and this goes against out code of conduct

@qwIvan
Copy link
Contributor Author

qwIvan commented Jan 18, 2018

deleted concat function

@schalkneethling
Copy link
Contributor

@qwIvan Thank you for the updates. It is looking good. I have run into one problem where the value undefined is not output. Have not debugged it to see what is causing it. Feel free to look into that. I will also look into and let you know.

The example I saw that with is weakmap-prototype-get.html

@qwIvan
Copy link
Contributor Author

qwIvan commented Jan 19, 2018

Convert undefined to String and resolved.

@schalkneethling
Copy link
Contributor

Convert undefined to String and resolved.

That is indeed the simplest solution, I wonder though, do we want to output it as a string? It is not a String under the hood, and is generally not output as a string. With that said, the solution to this will add additional code(to loop over the final array and replace 'undefined' with undefined when writing the output) and complexity. I am wondering if it is worth the effort.

@wbamberg Your thoughts?

@wbamberg
Copy link
Contributor

I see this:

// input
var f;
console.log("Hello", "World", f)
// output
> "Hello" "World" undefined

This looks fine, so unless there is some subtlety I am missing, I'm happy with it :).

Copy link
Contributor

@schalkneethling schalkneethling left a comment

Choose a reason for hiding this comment

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

r+

@schalkneethling schalkneethling merged commit f74fea2 into mdn:master Jan 24, 2018
@welcome
Copy link

welcome bot commented Jan 24, 2018

Congrats on merging your first pull request! 🎉🎉🎉

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