Skip to content

Conversation

@SimenB
Copy link
Contributor

@SimenB SimenB commented Sep 10, 2015

Fixes #35

Testing it became quite hard, so ended up introducing logger option, which is provided a sinon mock from the test

README.md Outdated
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't that be console.log.bind(console)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are absolutely right!

@lazd
Copy link
Owner

lazd commented Sep 13, 2015

I'm not sure about writing to stdout directly, I think we should use gulp-util's log() method in order to be a good citizen...

Also, I wonder if we need to expose an option at all for the logger. For now, wouldn't it just make sense to call gutil.log()?

@SimenB
Copy link
Contributor Author

SimenB commented Sep 13, 2015

gutil.log is just console.log with the time before it, isn't it? The whole point here is to not use console.log, as it adds a line break after each invocation. And joining the string apparently explodes if it's long enough

@lazd
Copy link
Owner

lazd commented Sep 13, 2015

You're right, process.stdout must be the right thing to do. Giving an option for the logger lets folks do whatever they want with the output, so they can avoid printing to stdout if that's undesirable.

@SimenB
Copy link
Contributor Author

SimenB commented Sep 13, 2015

I could maybe use gutil.log in the example in the readme instead of console.log?

A thing that just hit me, we could go the other way around, and have a big message saying "if you get RangeError: Invalid string length, use process.stdout" followed by an example. Then keep using gutil.log as the default

@lazd
Copy link
Owner

lazd commented Sep 13, 2015

Yeah, showing gutil.log might be nice, as the [gulp] prefix and whatnot may be expected by some users. On the other hand, it may interfere with the report output. It's up to you on this one.

@SimenB
Copy link
Contributor Author

SimenB commented Sep 13, 2015

@lazd How about now?

I also added an example if you want to print to file. I think this satisfies #34, and I'm closing it if this is acceptable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This small note could be removed when #29 comes in, dropping support for custom

@SimenB
Copy link
Contributor Author

SimenB commented Sep 27, 2015

@lazd Thoughts?

@lazd
Copy link
Owner

lazd commented Sep 29, 2015

Let's merge this now, then quickly get #29 together, then do a release. That way all the reporter stuff happens at the same time. Sounds right, @SimenB?

@SimenB
Copy link
Contributor Author

SimenB commented Sep 29, 2015

Aye, sounds good

By default, use process.stdout.write to print the report

Fixes #35
Closes #34
@SimenB
Copy link
Contributor Author

SimenB commented Feb 28, 2016

@lazd Rebased this

@lazd
Copy link
Owner

lazd commented Feb 28, 2016

@SimenB if we're good to go here, I can merge this. Do you want to do #29 immediately after and push a release or should we wait on that?

@SimenB
Copy link
Contributor Author

SimenB commented Feb 28, 2016

This is done, yes.

Regarding @29 I'd like to get it done, but I'm quite busy the next couple of weeks, and won't be able to wrap it up until after that

@lazd
Copy link
Owner

lazd commented Feb 28, 2016

Ok, I'll go ahead and merge/release this as a patch release.

@29
Copy link

29 commented Mar 2, 2016

Huh?

@lazd
Copy link
Owner

lazd commented Mar 2, 2016

@29 sorry, I believe we meant #29 :)

@SimenB
Copy link
Contributor Author

SimenB commented Mar 2, 2016

Oh yeah haha. Sorry about that!

@SimenB
Copy link
Contributor Author

SimenB commented Mar 29, 2016

@lazd ping 😄

@lazd
Copy link
Owner

lazd commented Mar 30, 2016

@SimenB sorry for the delay, merging now!

@lazd lazd merged commit 122cfe8 into lazd:master Mar 30, 2016
@lazd
Copy link
Owner

lazd commented Mar 30, 2016

Released in 0.3.0!

@SimenB SimenB deleted the print-huge-strings branch March 30, 2016 04:15
@SimenB
Copy link
Contributor Author

SimenB commented Mar 30, 2016

Sweet! I'll try to wrap up #29 ASAP then, and a 1.0 release should be GTG 😄

@lazd
Copy link
Owner

lazd commented Mar 30, 2016

@SimenB awesome!

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