PR: slice(styled, begin, end)

[StreetStrider]

I've started to work on tests for #27. I will push commits to corresponding branch, so this PR will be live.

What do you think about adding also length in support? It will calculate styled string's length. I was thinking about slicing styled strings and sometimes this will require acquiring string's length. The implementation (for now) will be simple strip(styled).length.

[medikoo]

@StreetStrider looks as a great start. length sounds ok to me, I would probably name it get-stripped-length.js (accessible and clc as clc.getStrippedLength(str))

[StreetStrider]

I should proceed working on tests. Some of them are too smart for my current implementation. i.e the output will have "empty" styled chunks (so test will fail), but styling will be correct (so actually everything is ok). I'll also do a linter pass after everything will settle down.

[StreetStrider]

@medikoo, it seems that lint rules does not have one for using var's. I'm personally using one var per variable. However, in your code I saw several styles, most of the cases combining var's in one block with one var. How should I tidy my var's?

[medikoo]

@StreetStrider Lint definitely won't allow var's in declaration blocks (like if's), and that's very valid, let's not do that. Concerning other things my style is as following:

var foo = require('...')
  , bar = require('...')

  , lorem = x, somethingElse = y
  , other, not, assigned, vars;

If you're troubled by comma first, I use it only for var declarations, and it's ok for me if you would prefer to not follow that (e.g. because of problem with editor configuration)

When speaking of one var block, it's fine to me to create another block of vars in some other part of function, but it should be rather about different logical blocks that are grouped (I wouldn't like to have vars spread everywhere) so there's e.g.:

// Functionality A
var foo, bar....

//.... code that covers A

// Functionality B
var fooB, barB...

//.. code that cover B

If that's not allowed at this point, you may add vars token to /.lint file, then lint will stop reporting about that

[StreetStrider]

@medikoo, the last error which present in linter output is «function using before its definition». Why should functions be defined upper the place they used? I'm asking, because I'm using the opposite style and this brings advantages. Consdier the code:

fn A() { .. uses B() .. uses C() .. }
fn B() { .. uses C() .. }
fn C() { .. }

In such style I can read code from things that really matter (function A) to less important things (B, C) from up to down, like we usually read text. In my code the most important thing is slice function itself, which is exported. tokenize, sliceSeq are not so important and placed after export. So when someone will have to read this, he will start from the function he interested and then will proceed to function's components. This is just a question.

[medikoo]

@StreetStrider I follow convention, where no function declarations are allowed. It's simplification, to not rely on hoisting, and understand function as any other object. The simpler the rules the less is confusion :)

[StreetStrider]

@medikoo, it's ready to review now. But I'm not sure the job is done. I want to do two things:

1. I've started to use this feature in one private project, I want to give it a time to «battle-test» itself.
2. I want to proceed to work on some advanced tests. Maybe you have ideas what is worth to be tested here. I'm sure not all test cases are covered.

[StreetStrider]

@medikoo

where no function declarations are allowed

Sorry, do I need to replace function foo() {… with var foo = function() {…?

[medikoo]

Sorry, do I need to replace function foo() {… with var foo = function() {…?

Yes function foo is function declaration, while var foo = function () is function expression, and it's only latter that is used in this project

[medikoo]

@StreetStrider it looks very promising at first glance. Tomorrow morning (europe time) I'll give it a more detailed look, and if it'll appear to me as ultimately working solution, I'll be definitely happy to merge it and publish.

One question for now: Why two of the tests are commented out? Is this something that should work but for some reason doesn't?

[StreetStrider]

@medikoo

Is this something that should work but for some reason doesn't?

That is what I wanted to discuss. It is working, but I'm not sure which is the «correct» way it should work. Consider the next case: slice('AAA<red>BBB</red>', 0, 3). What the result shoud be? (1) 'AAA<red></red>' (2) 'AAA' The (1) preserving all CSI and it is my current strategy. The (2) is also looking good, but this will require a smarter implementation. Note that in both cases the output will be the same. The (2) strategy is also can be unreliable in such situation: slice('AAAA<reset>B', 0, 3) where we cannot just chop reset CSI. But I'm not sure is there any cases where (1) is not a correct strategy and (2) is.

[medikoo]

@StreetStrider I see, it's a question of how to treat non-range instructions (like erase, move etc).

First question is whether we want to either (1) support them in some way, or (2) treat those instructions as something that doesn't make sense with slice, and ignore them.

Deciding on which way we choose, I propose following implementation

• in case of (1), we may go mathematic way, and make implementation of slice a bit more sophisticated, via support of open and close ranges:

var str = 'AAA<reset>B';
slice(str, 0, 3); // AAA<reset>
slice(str, 0, 2.5); // AAA
slice(str, 3); // <reset>B
slice(str, 3.5); // B

Still I wouldn't include any ranges with that logic so:

var str = 'AAA<red>B</red>C';
slice(str, 0, 3); // AAA
slice(str, 0, 2.5); // AAA
slice(str, 3, 4); // <red>B</red>
slice(str, 3.5, 4); // <red>B</red>
slice(str, 4; // C

• In case of (2) we should just ignore any non-range instructions on edges, but preserve them if they appear in a middle of a string so:

var str = 'AAA<reset>B';
slice(str, 0, 3); // AAA
slice(str, 3); // B
slice(2, 4); // A<reset>B

// And we go same way as in (1) with ranges
var str = 'AAA<red>B</red>C';
slice(str, 0, 3); // AAA
slice(str, 0, 2.5); // AAA
slice(str, 3, 4); // <red>B</red>
slice(str, 3.5, 4); // <red>B</red>
slice(str, 4; // C

My proposition is that if (1) is not difficult to implement, then go with it. It's surely quirky to support open and close ranges in slice, but it doesn't break normal (expected) behaviour, and it gives full control over custom cases. Still if it's too much hassle, then just go with (2), it's great enough.

[StreetStrider]

@medikoo, looks like there are more edge cases here. The CSI on the edge of the slice is a issue, but I was speaking not only about it.

I can formulate my original issue in more generic manner:

Can we remove any CSI if it does not fall into the slice and still have correct styles?

Here's the more realistic example: I have a column in table: '<any styles…>column content<reset>' and then I want to limit column's width: slice(value, 0, max). As reset is at the end of the sequence, it would be chopped in case when column is too big, breaking styles of following columns.

As for CSI at the edge of slice:

1. If I slice subrange of a <red> range I want it to be red too. (Just like copy-paste of styled text. Like '<red>123</red>' — here the 2 will be red just like 1 and 3, so when I slice it, I want it to save its styling.)
2. If I slice a subrange that starts with <red> and red ends with it, I want it to be red too.

I'm ok with support for open ranges, but I don't see how it can be applied:

1. It don't break ordinary styles: AAA and AAA<red></red> will look the same.
2. As for reset and others I was saying above. AAA and AAA<reset> is a very different behavior.

[medikoo]

As reset is at the end of the sequence, it would be chopped in case when column is too big, breaking styles of following columns.

I understand that by <reset> you mean reset to default colors operation (initially I thought it's about clear screen). In such case we speak just about ranges.

I believe all produced slices should come with end of range CSI for every range that were opened and not closed. And I think it would be better to discard empty ranges at the end of produced slice (so produced strings is as clean as it could be)

[StreetStrider]

@medikoo, So I will need to expose mods, correct?

[medikoo]

Yes, if you'd like to have this objects accessible in this util, then good idea is to seclude it to lib folder

[StreetStrider]

I've thinked about how slice should work, and I've agreed with your position. Added tests for it. So:

1. slice chunk and capture proper styles for it (both inner and outer)
2. reduce all styles which not captured in slice
3. reduce all control CSI sequences

But what I was talked about has sence too, but it is another strategy. It is like «chop» or something like that. It just chops substring and prevents any styles from missing. This works good if there's clc.reset at the end of the string: slice won't capture it, and chop will. On the other hand it will not work properly with move CSI sequences where slice will be ok.

[medikoo]

Is chop something as regular slice, but working on resolved string length, and assuring that chop doesn't happen in middle of some control sequence? If so what would be the use case for it?

Or is it about something I proposed in comment above so dev can decide whether on edge non range control sequences should be included in returned slice?

[StreetStrider]

The main case for me is to limit long string to some column's width. Maybe there's also similar cases, like constructing complex UIs (but not enough complex to involve ncurses), where you need to output labels, columns, text blocks. Maybe splitting styled text into lines (I'm not sure here, slice could be enough for it).

I think the most problematic CSIs here are reset and moves, if avoid it slice will be enough. I'm trying to avoid it.

[medikoo]

The main case for me is to limit long string to some column's width.

But I understand that slice as we designed it, addresses that perfectly? What would be use case for chop then?

I think the most problematic CSIs here are reset and moves, if avoid it slice will be enough. I'm trying to avoid it.

By avoiding you mean, clearing it up from the slice? I think slice should be agnostic that (as we described it above), as in normal circumstances you wouldn't put string containing such CSI to it.

If there's a risk that strings you handle may contain them, in first place I would trim them from such CSI, and for that what might be helpful, is a stripper that strips only non-range (so move, erase etc) CSI from given string, but leaves any formatting, colors intact. Having that such string should be first stripped with it, then put for slice.

[StreetStrider]

By avoiding you mean, clearing it up from the slice?

No, I mean not using it:

you wouldn't put string containing such CSI to it

Yes, that one.

I'm OK with slice. I've already started using it. If some real issues occur we will be able to improve this feature. chop is just for corner-cases.

[medikoo]

So to me chop is actually a strip that will strip just specified CSI and not all of them (we may extend current strip to address that). Still If you don't need it now, I wouldn't bother with it.

[StreetStrider]

@medikoo, hello. Should I test internal files? In my case test framework forces me to create test for lib/sgr.js which is not a part of the interface. I've created empty test/lib/sgr.js for it (to suppress error), but not commited it.

[medikoo]

@StreetStrider technically yes, I write tests also for modules in lib (even though not considered as front for public API), still if you feel that most is covered in other test file, then just add there few simple sanity tests.

[StreetStrider]

@medikoo, io throbber tests failed for some reason. Can you re-run them in such cases? I think everything is ok on my end.

As for everything other, I think PR is ready for review. slice is now working as we've discussed. I've also added very simple tests for lib/sgr.

[medikoo]

@StreetStrider looks as great work!

I've just put few comments to clean things a bit, and I look forward to merge it and publish it :)

[medikoo]

Great thanks! Published as v1.1.0

[StreetStrider]

Thanks for documenting it.

If some issues occur, call me in.