medikoo / cli-color

Colors and formatting for the console
ISC License
676 stars 34 forks source link

[PR] nested style support, .trim/.throbber inline and improvements #20

Closed rentalhost closed 9 years ago

rentalhost commented 9 years ago

I did worked on a lot of features that I suggest here #17 and here #18.

NOTE

It have some problems with lint and strange characters outputing on tests. I'll work on it tomorrow. I don't have linux here, but I'll install it on virtualbox to make tests again.

It implements:

About parser changes (index.js):

Feel free to implements part on 0.4.x and part on 1.0.x if you wants. I hope that you like it. Tell me if exists something that you don't like. I guess that will not be a problem if you launch all as minor version, because not is expected that users use nested color, one time that it was not supported before, and basically reset to default color.

Tested on: Windows 8.1 Console (cmd) and Cygwin (to emulate xterm)

medikoo commented 9 years ago

@rentalhost great thanks for great work. It contains a lot of good things, I would definitely like to take.

Still let's first assure that Travis CI passes, and please see my comments direclty in code

StreetStrider commented 9 years ago

Hmm, what do I wrong?: clc

The tests are passed.

rentalhost commented 9 years ago

@StreetStrider Seems that you not does nothing wrong. I'll check this on next weekend. Thanks. :)

rentalhost commented 9 years ago

@StreetStrider how are you doing your tests? Seems that all is Ok, in this aspect.

StreetStrider commented 9 years ago

@rentalhost, yes the tests are green. But they do not correspond proper colors. In my screenshot I run commands copypasted from tests. And they output exactly as in tests. But this do not lead to proper coloring. I suppose tests are wrong, I do not see any color-restoring in them after previous color ends.

How I do the tests: First I git clone your cli-color copy, then npm install all deps. Then do npm tests.

rentalhost commented 9 years ago

Ah... Now I see... Seems that the the second red color are not restarting. Your test seems like this:

    a(t.red('red ' + t.blue('blue ') + 'red'),
        '\x1b[31mred \x1b[34mblue \x1b[39m\x1b[31mred\x1b[39m',
        "Nested Foreground: Two Levels Type 3");

But if you look nearly, you will see that the red color is restarted normally. It's very strange. Seems that you are running the current cli-color version, and not this PR. Can you check it?

StreetStrider commented 9 years ago

@rentalhost, no, I was using your feature-branch. Take a look at screenshot. There's a revision in terminal: aba6ea5. Is it correct or you have other commits on top of it? I can test again.

rentalhost commented 9 years ago

There is not, currently. In anyway, I'll change some codes after. So you can do new tests to check it, probably on weekend. What is your OS and how are you running your tests? Directly on default OS terminal?

StreetStrider commented 9 years ago

@rentalhost, please, ping here when I should retry tests. I'm using xfce4-terminal. This is the default terminal on my system. Shell is zsh. But I don't think this has anything with terminal or shell. Some of my stuff depending on cli-color and it works flawlessly.

rentalhost commented 9 years ago

I'll back to work on this PR on weekend (tomorrow or after). So I make new tests and you retry it. For now, I'm working :(

rentalhost commented 9 years ago

@StreetStrider Could you try it with original version of cli-color?

clc.red("red " + clc.blue("blue " + clc.red("red")));

rentalhost commented 9 years ago

@StreetStrider can you repeat your tests now?

@medikoo can you fix the columns.js error? I just think that you need publish a new version to es5-ext. I added tests for this feature and I created clc.columns() too.

StreetStrider commented 9 years ago

Here's the tests: 1.301 517 Ok [98.85%] 3 Failed 3 Errors done on revision ca4cf71. Failed tests: columns, throbber and trim. Nested colors passed successfully. I found that color problem, that was on my side. For now, nested colors works properly. Nested colors tests are OK.

rentalhost commented 9 years ago

@StreetStrider great. What was the color problem? You can explain me? About failed tests, what was the tests failed exactly?

rentalhost commented 9 years ago

Seems that it is working perfectly now.

Ubuntu (xfce4-terminal, zsh, xterm): image

Windows: image

But I noted that Unix when you set a background, it change default foreground color to a bright color too. Is like I do:

clc.bgRedBright("red"); 
// On Windows is equivalent to: <bgRedBright><white>red</white></bgRedBright>
// On Unix is equivalent to: <bgRedBright><whiteBright>red</whiteBright></bgRedBright>

It create an inconsistency between systems. But I don't know if it is alarming.

image

StreetStrider commented 9 years ago

@rentalhost, the problem was in my custom repl. It caches previous version of cli-color in require.cache. I just run in node instead and everything works fine.

Errors:

23:11:28.964  -  columns.js
                 Error: Cannot find module 'es5-ext/iterable/validate-object'

23:11:28.972  -  index.js
                 Error: Cannot find module 'es5-ext/iterable/validate-object'

23:11:29.025  -  trim.js
                 Error: Cannot find module 'es5-ext/iterable/validate-object'

23:11:30.240  ✗  throbber.js: Formatted: No stderr output
                 Expected: ''
                 Actual:   '\nmodule.js:340\n    throw err;\n          ^\nError: Cannot find module \'es5-ext/iterable/validate-object\' ...

Revision: ca4cf71

rentalhost commented 9 years ago

This is because of an updated not published of es5-ext from @medikoo. You can fix it locally by get a copy from repo of this module.

@medikoo please, fix it before you publish this new version.

rentalhost commented 9 years ago

I created a new module called art(). It's useful to create graphical layouts based on text.

I think that need only publish the current version of es5-ext and all will be fine.


Currently it is the visual test:

image

medikoo commented 9 years ago

@rentalhost @StreetStrider great thanks, that looks very promising. This week I'll take a moment to look into it, and most likely we'll publish it as v1.0 (as it's good to leave 0.x versioning behind).

Concerning es5-ext, for now you may refer to it in package.json via git url, then tests should pass. Of course we won't publish it like that to npm.

StreetStrider commented 9 years ago

I worry about package size. The main reason is dependencies, of course.

4,9M    ./node_modules
3,9M    ./node_modules/es5-ext
936K    ./node_modules/memoizee
116K    ./node_modules/timers-ext
64K     ./node_modules/d

The package size has grown greatly.

rentalhost commented 9 years ago

5M is not a problem, because not all packages are loaded on this module, but only downloaded. Really, is possible reduce this a lot, as not use any package, but I guess that is not a good thing to do, once that external modules has your own tests units.

medikoo commented 9 years ago

Sorry for long delay, I finally had time to look into it, and I've taken all proposed features into master.

Firstly, sorry that I didn't take any of this work directly. I've done some bigger rearrangements within package, and non of the commits were applicable to take with cherry-pick. Anyway in each commit where I copied your work I mentioned you directly, and exposed you're effort in documentation. I hope that works well for you.

Before I publish it as 1.0, Please take a look in master and let me know what you think. List of changes:

StreetStrider commented 9 years ago

@medikoo, good point on function's grouping :+1: I'll try to use master.

rentalhost commented 9 years ago

@medikoo Great work! I just think that you don't need credit me in functions like .art() and nested style, just on footer is ok for me, and I think that will be better if you have new contribuitors on future, else, it'll overflow of "thanks" on readme. :)

medikoo commented 9 years ago

@rentalhost that's good point, we'll leave documentation clean, and I'll keep you in Contributors section :)

medikoo commented 9 years ago

It's published as v1.0.0. Thank you!