Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

would like to try this out in nushell #143

Closed
fdncred opened this issue May 19, 2022 · 156 comments
Closed

would like to try this out in nushell #143

fdncred opened this issue May 19, 2022 · 156 comments

Comments

@fdncred
Copy link
Contributor

fdncred commented May 19, 2022

hey @zhiburt,
i'd love to try your tabled crate out in nushell just to see how it stacks up and if it could work for us, what the performance is like, how it handles emojis, etc. are you interested and available to help with this?

if so, i'd see it initially working this way as a prototype. right now most commands automatically call table which creates the normal nushell tables. i figured we could create a new command called tabled and then just do ls | tabled and blah | tabled just to see how it works.

what are your thoughts?

@zhiburt
Copy link
Owner

zhiburt commented May 19, 2022

Hi @fdncred

what are your thoughts?

I was actually thinking about creating a binary of this kind a bunch of times.
Though I haven't came with the format which must be required by stdin.
What approach do you use in nushell?

I'd love to try your tabled crate out in nushell just to see how it stacks up and if it could work for us, what the performance is like, how it handles emojis, etc. are you interested and available to help with this?

Sure,
Personally the performance would be my biggest concern here. I want to believe the performance is OK; but who knows... It would be interesting to see how things are really going.
*We do a bunch of allocations on each `fmt::Display which ideally should be eliminated; (maybe #123 can help)

Then do you propose me to create a prototype?

@fdncred
Copy link
Contributor Author

fdncred commented May 19, 2022

Then do you propose me to create a prototype?

Yes, exactly. :) Feel free to say, "No!".

What I was thinking and hoping for was to create a tabled internal command inside of nushell just as there is a table internal command inside of nushell. I wouldn't want to use an external command because I don't think it would work right.

My biggest concern is that we truncate columns with ... sometimes and we also wrap text when there isn't enough space. I'm not sure if tabled supports either of those yet. But it still may be worth a try to look at any internal command that uses the input variable and see if a tabled command could take that input, figure out the column names and create rows, just as a prototype.

@zhiburt
Copy link
Owner

zhiburt commented May 19, 2022

My biggest concern is that we truncate columns with ... sometimes and we also wrap text when there isn't enough space. I'm not sure if tabled supports either of those yet.

Must be supported.

Better look at my concern 🥲 😥

Screenshot_2022-05-20_01-24-41


Seems like a lot of allocations.

@zhiburt
Copy link
Owner

zhiburt commented May 19, 2022

A bit Improved allocations, but it still high.

Screenshot_2022-05-20_02-41-36

Screenshot_2022-05-20_02-38-25

@fdncred
Copy link
Contributor Author

fdncred commented May 20, 2022

nice

@zhiburt
Copy link
Owner

zhiburt commented May 23, 2022

(surprisingly) Getting closer.

Screenshot_2022-05-24_00-12-33

@fdncred
Copy link
Contributor Author

fdncred commented May 25, 2022

how exactly are you generating these cool graphs?

@zhiburt
Copy link
Owner

zhiburt commented May 25, 2022

Well it's not me.
All the credit to the https://docs.rs/criterion/latest/criterion/
(apparently it uses gnuplot?)

Also see #139

I've put there a few more

@zhiburt
Copy link
Owner

zhiburt commented May 27, 2022

Hi @fdncred

I've created a small patchwork here https://github.com/zhiburt/nushell/tree/checkout-tabled.
You can check this out.

I did not made any refactoring and stuff like this;

As I see there's a difference in logic of alignment to terminal width.
Current logic I guess is not applicable for nu_shell.
So it may deserve a change.

image

image

@fdncred
Copy link
Contributor Author

fdncred commented May 27, 2022

oh, nice. I'm shocked at how few changes it took to hook up tabled to nushell. nice job.

I noticed that it doesn't have the footers that nushell has. I'm running it now. it's pretty fun.

some of the coloring is different too (the lines). maybe tabled doesn't support that yet.

numbers aren't left-aligned but I'm pretty sure tabled supports that.
nushell tables
Screen Shot 2022-05-27 at 6 28 15 AM
tabled tables
Screen Shot 2022-05-27 at 6 28 45 AM

@fdncred
Copy link
Contributor Author

fdncred commented May 27, 2022

This alignment/wrapping is interesting too, like you already mentioned.
Screen Shot 2022-05-27 at 6 34 04 AM

Screen Shot 2022-05-27 at 6 34 20 AM

@fdncred
Copy link
Contributor Author

fdncred commented May 27, 2022

and oh, I finally understand your second screenshot. you made the terminal smaller and wow that makes columns kind of disappear. very interesting.

it's still cool though, you're doing a great job with tabled. Congrats! I wonder if we could/should try to make tabled work more like nushell with alignment and wrapping?

@zhiburt
Copy link
Owner

zhiburt commented May 27, 2022

I noticed that it doesn't have the footers that nushell has

We could do it by pushin headers to the data.
Though to mimic the nu_table theme we would need create a custom ones to consider this last rows.

Am I right that nushell just prints headers and the bottom and the top?

ref: #126

some of the coloring is different too (the lines). maybe tabled doesn't support that yet.

Quite interesting.
Could you point out where they're different?

numbers aren't left-aligned but I'm pretty sure tabled supports that.

I did a right alignment

and oh, I finally understand your second screenshot. you made the terminal smaller and wow that makes columns kind of disappear. very interesting.

Correct


By the way I've just noticed that on one of your screenshots table has colored borders.
We are considered to support that but,
How do you do that?

I mean as I understand this theme is not in a default list.

@zhiburt
Copy link
Owner

zhiburt commented May 27, 2022

I wonder if we could/should try to make tabled work more like nushell with alignment and wrapping?

Couldn't we do both?
If it being exposed by config or in the args to table.

By certainly the default must be decided.


After some work; (not yet finished).

I feel like this is similar to your approach right?

image

@fdncred
Copy link
Contributor Author

fdncred commented May 27, 2022

Am I right that nushell just prints headers and the bottom and the top?

ya, but there's a config setting in nushell that controls when to print it.
footer_mode: always #always, never, number_of_rows, auto

Could you point out where they're different?

in the two screenshots of the regular ls i have the lines/borders configured to print in purple on the nushell tables but in the tabled tables they print in white.

I'm using a custom theme to make the borders purple and change some other colors. I have this in my config.nu.

let my_theme = {
    separator: yd
    # leading_trailing_space_bg: red
    leading_trailing_space_bg: { attr: n}
    header: cb
    date: pu
    filesize: ub
    row_index: yb
    hints: dark_gray
    bool: red
    int: green
    duration: blue_bold
    range: purple
    float: red
    #string: red
    nothing: red
    binary: red
    cellpath: red

    # just take defaults for shapes
}

and then later on in the let-env config = { section I do

    color_config: $my_theme

@fdncred
Copy link
Contributor Author

fdncred commented May 27, 2022

Couldn't we do both?

we could probably have a config setting to use tabled versus nushell tables. I'm just wondering if tabled needs to support column truncation where we show ... as a column and the other columns are just truncated?

I feel like this is similar to your approach right?

I think it's looking better. size isn't right aligned.

It would be nice to have a side-by-side nushell table vs tabled table - it's hard to spot differences without comparing, but it's looking good!

@fdncred
Copy link
Contributor Author

fdncred commented May 27, 2022

oh, i almost forgot, we have this option too on nushell tables to turn off the first column.

    disable_table_indexes: true # set to true to remove the index column from tables

@zhiburt
Copy link
Owner

zhiburt commented May 27, 2022

I'm just wondering if tabled needs to support column truncation where we show ... as a column and the other columns are just truncated?

So we can either truncate or wrap;
We don't do both;
We can but not out of the box.

See

image

ref: #152

@zhiburt
Copy link
Owner

zhiburt commented May 27, 2022

So yes there're quite a few things to do to cover the whole nu_table usage.

It would be nice to have a side-by-side nushell table vs tabled table - it's hard to spot differences without comparing, but it's looking good!

Don't say we need tests 😄

@fdncred
Copy link
Contributor Author

fdncred commented May 27, 2022

I'd like to be able to prove that tabled can replace nushell tables somehow, even if tabled doesn't have all the features. I'm wondering if we can make it configurable to use tabled in nushell. Either put it behind a feature or add a config.nu flag. At least then we can spot issues and log them on your repo.

If you're not really interested in doing this, that's ok too. I just really like tabled. :)

yes, we'd need tests somehow. lol.

@zhiburt
Copy link
Owner

zhiburt commented May 30, 2022

I tend to think I've covered most of the mentioned things?

You can check this out on the same branch https://github.com/zhiburt/nushell/tree/checkout-tabled.

image

@zhiburt
Copy link
Owner

zhiburt commented May 30, 2022

1 thing which I didn't figured out is why on your screenshot size column has a right alignment when it's not an index, maybe you can clarify it?

image

@zhiburt
Copy link
Owner

zhiburt commented May 30, 2022

todo: handle the case when table can't be rendered because not enough space.

@fdncred
Copy link
Contributor Author

fdncred commented May 30, 2022

IIRC, the way it works in nushell is that data types are assigned an alignment. It's in the color_config.rs https://github.com/nushell/nushell/blob/f5519e2a0958112ce371fcab57b4bdbbc8f929ef/crates/nu-color-config/src/color_config.rs#L234-L385

@zhiburt
Copy link
Owner

zhiburt commented May 30, 2022

IIRC, the way it works in nushell is that data types are assigned an alignment. It's in the color_config.rs https://github.com/nushell/nushell/blob/f5519e2a0958112ce371fcab57b4bdbbc8f929ef/crates/nu-color-config/src/color_config.rs#L234-L385

Great;

image

@zhiburt
Copy link
Owner

zhiburt commented May 30, 2022

Also added the support for the notion if we can't print table.

image

@fdncred
Copy link
Contributor Author

fdncred commented May 30, 2022

nice job. are these changes in your latest checkout-tabled branch? I'll try to try later if so.

@zhiburt
Copy link
Owner

zhiburt commented May 30, 2022

are these changes in your latest checkout-tabled branch? I'll try to try later if so.

Yes they are.

Take care then :)
til later

@fdncred
Copy link
Contributor Author

fdncred commented Jun 2, 2022

looking very nice! good work.

the next challenge, if you choose to accept it, is to get this working a good as possible. Run your tabled version of nushell and open this file. Scroll around and look at all the column lines that are drawn wrong.

To be clear, this is a big problem in nushell and I'm not sure if there even is a "fix" for it since it depends on how each terminal renders emojis.

I'm also interested in caching, maybe related to above. for instance, read the first 1000 lines of the emoji file (or other files) before deciding how wide to set the columns. so, this caching isn't necessarily for speed but for determining proper column width.

@fdncred
Copy link
Contributor Author

fdncred commented Jun 2, 2022

one of the core team members asked if string truncation is possible in tabled. for instance, if you're running nushell and type env you get a large table and the config items is pretty big. It may be nice to only show the first xx chars of any item. is that possible with tabled?

@fdncred
Copy link
Contributor Author

fdncred commented Jun 30, 2022

This is weird too. Just moving the 2; from foreground to background
image
without dim it looks like this, which is correct.
image

@fdncred
Copy link
Contributor Author

fdncred commented Jun 30, 2022

wow, this stuff is so confusing/hard/weird

@zhiburt
Copy link
Owner

zhiburt commented Jun 30, 2022

Just verified;

This 2; somehow influence parsing; We don't parse 1 sequence as ansi but treat it as a text.
Meaning; the issue may be a bit dipper the the buffer size.

Just checked;

It's just not expected.

@panicbit
Copy link

panicbit commented Jun 30, 2022

I'd suggest checking weird behaviour in different terminals to rule out quirks in your current terminal.

@zhiburt
Copy link
Owner

zhiburt commented Jul 1, 2022

@fdncred https://github.com/zhiburt/tabled/blob/master/examples/README.md

I'd suggest checking weird behaviour in different terminals to rule out quirks in your current terminal.

Hi @panicbit I think it's right approach.
But as I understand most (or all) former mentioned issues are caused by a ansi parsing issue.

[WIP]

@fdncred
Copy link
Contributor Author

fdncred commented Jul 1, 2022

@fdncred https://github.com/zhiburt/tabled/blob/master/examples/README.md

whoop! whoop! that looks great!

@zhiburt
Copy link
Owner

zhiburt commented Jul 3, 2022

Just keep you posted.

So it's still WIP;
And I haven't done testing almost at all.

PS: If you have any 'corner cases' let me know; so I could check it.


wrap "\e[2;48;5;10m\e[38;5;20mDar\nren\e[0m"

image

wrap "\e[48;2;127;0;255;38;2;255;255;0mDar\nren\e[0m"

image

wrap "\e[48;2;127;0;255;38;2;255;255;0mDar\nren\e[0m"

image

@fdncred
Copy link
Contributor Author

fdncred commented Jul 3, 2022

Looking good. Thanks for the update.

@fdncred
Copy link
Contributor Author

fdncred commented Jul 3, 2022

What's the difference between the last two? What about adding dim like 2; before the foreground one and/or the background one?

@zhiburt
Copy link
Owner

zhiburt commented Jul 4, 2022

Probably I did a mistake when do copy&paste.

But I've updated https://github.com/zhiburt/nushell/tree/nu-table-to-tabled branch.
So you can check it out yourself.

@fdncred
Copy link
Contributor Author

fdncred commented Jul 4, 2022

@zhiburt very nice work!

@fdncred
Copy link
Contributor Author

fdncred commented Jul 5, 2022

@zhiburt head's up. we're releasing 0.65.0 sometime today. then we'll wait for a day or so looking for bugs that slipped in. after that window I'd like to land your tabled PR in nushell so we can start using tabled for ~3weeks testing it. Is that ok with you? You might get some bugs filed and such that we'd need to fix in the next few weeks if we decide to keep tabled.

@zhiburt
Copy link
Owner

zhiburt commented Jul 5, 2022

Sounds good;

Although I have 1 concern; is a wrap logic;

Originally while printing nu-table reuse lines and width data which it takes when it does wrap.
With tabled we do split string to lines once in wrap logic and a second time when displaying.

I just want to highlight it once again;
As we do quite a few redundant iterations and allocations because of that.

ref: #97

BTW: And wrap.rs is pretty complex so it may deserve a try to be refactored.

@zhiburt
Copy link
Owner

zhiburt commented Jul 5, 2022

Seems like we do less of them any wayfor turtle 5. (I guess because it has not a lot of multi line strings) (and we probably just allocate bigger chunks)
But we do more then enough to say it's a lot.

tabled
image

nu-table main.
image

So 2k allocations is presumably done in wrap

PS: It's all synthetic assumptions any way.

@fdncred
Copy link
Contributor Author

fdncred commented Jul 6, 2022

whenever you're ready to push a PR, we can probably land it on 0.65.1 and start testing.

@zhiburt
Copy link
Owner

zhiburt commented Jul 6, 2022

I hope you meant me to open it;
So here it is nushell/nushell#5969

@fdncred
Copy link
Contributor Author

fdncred commented Jul 6, 2022

I hope you meant me to open it;
So here it is nushell/nushell#5969

Yup. That's what I meant. Just need to fix the clippy lints and we can probably land it.

@fdncred
Copy link
Contributor Author

fdncred commented Jul 6, 2022

@zhiburt we have our first bug report. We're setting up a tabled label in our issues so we can easily find them. Our first issue is that 0..100000 takes 10x longer than nu-table. As soon as the label and issue is setup, I'll ping you here.

@fdncred
Copy link
Contributor Author

fdncred commented Jul 6, 2022

Here's the issue nushell/nushell#5972. Note that you can search by label too in order to see all the "tabled" issues in the future.

@fdncred
Copy link
Contributor Author

fdncred commented Jul 16, 2022

Just an update, everyone is loving the speed of tabled now in nushell. We're planning on keeping it for the next release. You probably know this but I wanted to mention it anyway. In order to publish, we'll need all the crates we're using published in crates.io. i.e. we can't have dependencies that point to git. So, if you don't mind, sometime before our next release, can you please publish the crates we're using? Our next release is on 26th of July.

@zhiburt
Copy link
Owner

zhiburt commented Jul 16, 2022

Hi

Just an update, everyone is loving the speed of tabled now in nushell. We're planning on keeping it for the next release. You probably know this but I wanted to mention it anyway.

Actually I didn't know what you've lean to.
That's great to hear.

I hope it will not be very error prone (in terms of issues).

In order to publish, we'll need all the crates we're using published in crates.io. i.e. we can't have dependencies that point to git. So, if you don't mind, sometime before our next release, can you please publish the crates we're using? Our next release is on 26th of July.

It's actually something I started to prepare for a last days.
I'll polish documentation a little bit and release it in short.

Hope you have a good weekend.

@fdncred
Copy link
Contributor Author

fdncred commented Jul 16, 2022

@zhiburt We had a request. It's not an emergency but something to think about. When using the truncation_suffix, would it be possible to color it the same color as what proceeded it. For example, here's some headers that I just created. The text is green but the truncation suffix is white.
Screen Shot 2022-07-16 at 4 05 40 PM

It's actually something I started to prepare for a last days.
I'll polish documentation a little bit and release it in short.

Thank you very much.

Hope you have a good weekend.

Thanks for that.

@zhiburt
Copy link
Owner

zhiburt commented Jul 17, 2022

@zhiburt We had a request. It's not an emergency but something to think about. When using the truncation_suffix, would it be possible to color it the same color as what proceeded it. For example, here's some headers that I just created. The text is green but the truncation suffix is white.

Created some snippet for it.

image

@fdncred
Copy link
Contributor Author

fdncred commented Jul 17, 2022

ooooh, that looks nice!

@fdncred
Copy link
Contributor Author

fdncred commented Jul 18, 2022

@zhiburt it appears that we have a regression. Lists now are acting like tables again by putting the first element into a header.

> [1 2 3]
╭───┬───╮
│ 0 │ 1 │
├───┼───┤
│ 1 │ 2 │
│ 2 │ 3 │
╰───┴───╯

image

@zhiburt
Copy link
Owner

zhiburt commented Jul 18, 2022

if line.is_empty() {

Was caused by this line I've added.
When doing refactoring I believe.

Mistake...

The said thing is it was not covered by tests.

@fdncred
Copy link
Contributor Author

fdncred commented Jul 18, 2022

no worries, thank you for fixing it!

@fdncred
Copy link
Contributor Author

fdncred commented Jul 26, 2022

heads-up. we're publishing nushell 0.66.0 today with tabled 🎉. thanks for all your excellent help on getting this done. i'm closing the issue for now. i have another question but i'll open a separate issue on that.

@fdncred fdncred closed this as completed Jul 26, 2022
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

No branches or pull requests

3 participants