Issue#297 resolve cli table dependency #311
Conversation
|
This is working in node v10 but still failing in node v8 per https://travis-ci.org/zemirco/json2csv/jobs/397145497 (expected doesn't have colors but actual does), I will chase that one down. |
|
I've tested locally for 8. I was assuming the test suite should be the same for v8 and v10 (hence I didn't test with v8 previously my bad); the problem with my assumption is that I noticed the node version detection wasn't correct in the fork I forked from (https://github.com/schalkneethling/json2csv/blob/91cbe4fb023d35749221775eed65184909dc519d/test/CLI.js#L720 checks only first digit so returns 1 for node v10.x.x) but is fixed in zemirco/json2csv master branch. |
* specify this in package.json too * also can remove check for node < 8 on some tests as they work on v6 and v7
…-cli-table-dependency
|
Per discussion on #302 I've removed v4 and v5 from the .travis.yml list and also specified the requirement for node >=6.0.0 in the package.json file. With that chaange, and since the other work done on style fixes the other broken tests for node v6 and v7, I've now removed the check for node < v8 in CLI.js so that all tests are run (and now pass after installing and testing against different node versions locally and also in travis https://travis-ci.org/zemirco/json2csv/builds/397153027?utm_source=github_status&utm_medium=notification ). |
|
@matthewberryman I'd love to keep colors normally, but disable them for tests. Think you could give that a try as well? |
|
@knownasilya Yes that's a good point. I'll give it a go. |
(useful in testing, possibly other use cases too)
|
@knownasilya 810ca51 adds a new command line option for pretty formatting without colour, that behaves as expected (running at CLI on some json I have, plus tests), with --pretty still adding color |
|
I don't think that we should have two pretty print options. In fact, I don't think that we should have a pretty print option at all. We should have a separate library to do For now, I'd rather make the cli-table an optional dependency and let the user bring either Btw, there are many changes unrelated to the PR (spaces mainly). Those are good changes but would be better as a separate PR. |
| @@ -35,6 +35,7 @@ program | |||
| .option('-a, --include-empty-rows', 'Includes empty rows in the resulting CSV output.') | |||
| .option('-b, --with-bom', 'Includes BOM character at the beginning of the csv.') | |||
| .option('-p, --pretty', 'Use only when printing to console. Logs output in pretty tables.') | |||
| .option('-P, --pretty-without-color', 'Use only when printing to console. Logs output in pretty tables, but without color.') | |||
There was a problem hiding this comment.
this is unnecessary. cli-table3 through colors already supports a --no-color by default, so a dedicated option should not be needed here.
There was a problem hiding this comment.
The tests in CLI.js call child_process.exec on json2csv.js, so short of adding another command line option here to json2csv (to keep colour by default per #311 (comment) ) , or a fundamental re-write of the tests, this does seem necessary to pass that option through to json2csv as a process argument, because of the way it's called.
|
@juanjoDiaz I think removing the pretty print code in next major version bump makes sense, as its adding a lot of complexity and dependencies for a feature that I am guessing isn't used much, but in the mean time having cli-table3 in place solves several issues (this PR solves both #278 and #297 ). |
|
Closed in favor of #312 Thanks for the contribution and getting this moving. |
Supersedes #302 - fixes test issues caused by colors by setting the style parameter as @Turbo87 suggested in #302 (comment)