Save report to a file

[borisdiakur]

This pull request resolves #17 by hooking into process.stdout and writing the reporter output to a file. To do that the user can set one additional option outputPath. The path includes filename and extention (two options, reportFormat and reportFile, seemed redundant).
I also did some cleanup, hope you don't mind (see commits).

[stefanjudis]

@borisdiakur

Oh wow - that's what I call a PR. Nice one!

Will have a look right now. :) Thanks.

[stefanjudis]

You're creating a directory here, which won't be removed after test run.
We may want to remove it afterwards?

[danielstjules]

Right now fs.rmdir wouldn't run if any of the assertions failed. I'd suggest placing it in /tmp or the os tmp dir :) https://github.com/raszi/node-tmp

[stefanjudis]

👍

[stefanjudis]

@borisdiakur

Thanks for taking the time. A had a few notes - maybe we wanna discuss these.

The actual implementation is a bit hard to understand at the beginning - but I thought about it and probably wouldn't come to a nice solution. So this might be fine. :)

Maybe we can add some comments or something? I'm not sure about it.

Overall really good PR. Thanks for that.

Maybe @paazmaya can also have a look. :)

And yeah 👍 👍 👍 for some clean up!!!

[borisdiakur]

You're welcome. Actually I'd find it cleaner if the jsinspect module would offer the write to file part. That way we wouldn't have to use such a hack. Maybe we should send another PR to the other repo...

[stefanjudis]

Actually I'd find it cleaner if the jsinspect module would offer the write to file part.

Agree on that - maybe we wanna keep this one open, until @danielstjules decides that he would take it?

[danielstjules]

If I was using npm scripts or make, I'd just pipe the output: jsinspect . > yourfile Is there a clean alternative to this with grunt?

I guess one improvement would be to update the reporters to accept the stream to write to, rather than assume process.stdout.

[stefanjudis]

If I was using npm scripts or make, I'd just pipe the output: jsinspect . > yourfile Is there a clean alternative to this with grunt?

Hah, was expecting this reply. ;)

I guess one improvement would be to update the reporters to accept the stream to write to, rather than assume process.stoudout.

Might work yes.

What do you think about providing the reporter result with the end event?

https://github.com/stefanjudis/grunt-jsinspect/blob/master/tasks/jsinspect.js#L64-L66

E.g.

    inspector.on('end', function(result) {
      // do something with the result here

      done(taskSucceeded);
    });

[borisdiakur]

@danielstjules See danielstjules/jsinspect#24 - I'm about to fork your repo. Will create a PR.

[borisdiakur]

PR created: danielstjules/jsinspect#25
heading out for 🍕

[danielstjules]

I like the idea of accepting a writable stream. Makes integration tests super easy as well, since capturing stdout from mocha is a PITA :)

[borisdiakur]

PR danielstjules/jsinspect#25 has been accepted. Thanks @danielstjules!
Now we have to wait until the next release of jsinspect in order to rework and complete this PR.

[stefanjudis]

Good job @borisdiakur! 🎉

[danielstjules]

Just pushed a new release :) Please note that I renamed the option. Thanks for your patience, and all your help!

[stefanjudis]

👍 🎉

Nice example of open source collaboration! Love it!

[borisdiakur]

That was fast! Thanks again @danielstjules! Ok, I guess it's my turn now. Hang tight!

[borisdiakur]

The PR should be 👌 now. @stefanjudis: Your turn.

[borisdiakur]

tmp is nice!

[paazmaya]

Sweet 🍠 !

[stefanjudis]

okay looks good to me. :)