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

Make testing more robust on wide term size #114

Merged
merged 10 commits into from
Jun 18, 2022
Merged

Conversation

t-bltg
Copy link
Collaborator

@t-bltg t-bltg commented Jun 17, 2022

  • fix Testing broken (again) on some terminal sizes #112 (added comments in tests);
  • change testpanel and testtree function to macros, for better stacktrace when erroring;
  • rename truncate to str_trunc (since it shadows a builtin function, not a good idea);
  • swap remaining (w, h) in tests;

Fixes #112.

I've changed the "."^500 to "°"^500 instead in the panel tests because it initially got me confused with the added trailing dots ... when truncating the panel content.

@FedeClaudi, good to merge when green.

@codecov-commenter
Copy link

codecov-commenter commented Jun 17, 2022

Codecov Report

Merging #114 (5e35196) into master (72f1d23) will increase coverage by 0.89%.
The diff coverage is 96.07%.

@@            Coverage Diff             @@
##           master     #114      +/-   ##
==========================================
+ Coverage   94.12%   95.01%   +0.89%     
==========================================
  Files          33       33              
  Lines        1905     1907       +2     
==========================================
+ Hits         1793     1812      +19     
+ Misses        112       95      -17     
Impacted Files Coverage Δ
src/progress.jl 97.29% <ø> (ø)
src/tables.jl 98.80% <ø> (ø)
src/dendograms.jl 98.75% <50.00%> (ø)
src/trees.jl 92.92% <80.00%> (+0.81%) ⬆️
src/__text_utils.jl 91.01% <100.00%> (+0.10%) ⬆️
src/_progress.jl 97.97% <100.00%> (ø)
src/_repr.jl 90.90% <100.00%> (ø)
src/boxes.jl 98.21% <100.00%> (ø)
src/colors.jl 97.82% <100.00%> (-0.05%) ⬇️
src/grid.jl 100.00% <100.00%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72f1d23...5e35196. Read the comment docs.

@t-bltg t-bltg marked this pull request as draft June 18, 2022 07:58
@FedeClaudi
Copy link
Owner

Hey,

Finally had time to look into this. It looks good!
Why did you mark it as draft? I'm happy to merge when you are, I'll try to release v1.0 today.

Also, I think you've seen it but look at UnicodePlots and Term working together! https://twitter.com/mdcatchen/status/1537915624575209475?s=20&t=1Zl9bveIZlUyS7OF4FiJDQ

@t-bltg
Copy link
Collaborator Author

t-bltg commented Jun 18, 2022

Why did you mark it as draft?

I was reworking the grid slightly to accept a NamedTuple and use it's keys in the layout expression.

@t-bltg
Copy link
Collaborator Author

t-bltg commented Jun 18, 2022

I have a question regarding the colors used in Term.jl: it seems you are a bit re-inventing the wheel. Why no re-use the exhaustive Colors.jl or one of the numerous existing packages ?

E.g. using a style = "darker_red" does not work currently and requires FedeClaudi/MyterialColors.jl#7.

Also where did you get CODES_16BIT_COLORS or COLORS_16b from Term.jl/src/_ansi.jl ? It might be worth adding the sources / references...

@FedeClaudi
Copy link
Owner

Hey,

The colors lists for 8/16 bits colors are fairly standard: wikipedia, but I got the ones used here from rich: https://github.com/Textualize/rich/blob/master/rich/color.py

I have a question regarding the colors used in Term.jl: it seems you are a bit re-inventing the wheel. Why no re-use the exhaustive Colors.jl or one of the numerous existing packages ?

In part to have fewer dependencies for packages that seem to do a lot more. In part because functions like:

function get_color(string; bg = false)::AbstractColor

would still need to be there (e.g. to deal with background colors).
I'm guessing the main advantage would be the hex -> rgb conversion? But that's just a few lines I don't think it warrants having an additional dependency.

using a style = "darker_red" does not work

It doesn't because the colors in MyterialColors are not standard named colors (the 8/16bit colors listed above) They are nice colors that I use frequently so I made a package to store the hex codes to avoid having to remember/look up. Otherwise only 8/16 bit named colors are picked up from strings.

@FedeClaudi
Copy link
Owner

FedeClaudi/MyterialColors.jl#7

I'm confused about how this helps? You've improved how the codes are defined and removed some exported colors, but I don't see why `style="red_darker" would work after that?

@t-bltg
Copy link
Collaborator Author

t-bltg commented Jun 18, 2022

8/16 bits colors

I guess we're talking about 8-bit colors and 24-bit colors (true color, 1 byte = 8 bits per channel, for R, G & B) ?

In part to have fewer dependencies for packages that seem to do a lot more.

I see.

I got the ones used here from rich

Thanks.

removed some exported colors

No, they are still exported using eval https://github.com/FedeClaudi/MyterialColors.jl/pull/7/files#diff-1a9b9d7673a79fd4abb7a34d0effc0efe76c016905a16c1cf39277b6d790d913R123.

I don't see why `style="red_darker" would work after that

It's hidden in this PR: https://github.com/FedeClaudi/Term.jl/pull/114/files#diff-b29208680d97c8a77bc9060b3377d8d0ee4180f2a48bbe1a8210292504128a1cR146.

@FedeClaudi
Copy link
Owner

I see, got it thanks. Good addition with directly parsing the MyterialColors colors. I don't think users would take advantage of this (I suspect they'll use their own favourite colors/names), but it can be convenient internally so that we don't have to have variables holding the color names.
Do you know if getting the value out the dict everytime would be slower/more allocating?

@t-bltg
Copy link
Collaborator Author

t-bltg commented Jun 18, 2022

I don't think users would take advantage of this

I can remove that, and use more standard color names.

https://github.com/Textualize/rich/blob/master/rich/color.py

I think copying this table is not a good idea: I doubt anyone will use sea_green2 directly, and we could maybe use default named W3 html colors https://www.w3schools.com/colors/colors_hex.asp](https://www.w3schools.com/colors/colors_names.asp). Then we could map names to 24 bit color codes directly for hex values (https://en.wikipedia.org/wiki/ANSI_escape_code#24-bit).

Did you consider using Crayons.jl (we use it in UnicodePlots for handling colors) ?

Do you know if getting the value out the dict everytime would be slower/more allocating?

Better use a NamedTuple since it's non mutable.

@t-bltg
Copy link
Collaborator Author

t-bltg commented Jun 18, 2022

@FedeClaudi, let's merge this PR when green, and discuss colors in another PR, so that it doesn't block #115.

@t-bltg t-bltg marked this pull request as ready for review June 18, 2022 11:40
@FedeClaudi
Copy link
Owner

Okay.

I didn't look into Crayons.jl, but let's table that for another discussion (a PR or a Discussion)

@t-bltg
Copy link
Collaborator Author

t-bltg commented Jun 18, 2022

For reference, I did the transition 8bit -> 24bit for capable terminals in JuliaPlots/UnicodePlots.jl#238, and it was far from easy 😆 .

The 24 bit table of Colors.jl lies here: https://github.com/JuliaGraphics/Colors.jl/blob/master/src/names_data.jl.

@FedeClaudi
Copy link
Owner

it should be the same colors listed in Term? Just as RGB instead of the ANSI code for the named color.

@t-bltg
Copy link
Collaborator Author

t-bltg commented Jun 18, 2022

It seems the names differ. Also, you loose information when using ansi color codes (8 bit) over true RGB colors.

You can merge this PR now and I'll open another one.

@FedeClaudi FedeClaudi merged commit c6a6bf2 into FedeClaudi:master Jun 18, 2022
@FedeClaudi
Copy link
Owner

Okay, if you think that it;s worth it we can look into adding Crayons.

@t-bltg t-bltg deleted the bug branch June 18, 2022 11:55
@t-bltg
Copy link
Collaborator Author

t-bltg commented Jun 18, 2022

Well at least it unifies a bit Term and UnicodePlots, and it allows you to toggle easily between 8bit and 24bit colors.

It also depends if you are ready to drop MyterialColors and rework the colors (I don't want to impose anything, but working with colors can be hard, and it's always better to use the same conventions, regarding that matter).

That being said, it's going to be a mess, and breaking. But I can do that now before 1.0.

@FedeClaudi
Copy link
Owner

I'll be honest, I think what you're saying makes a lot of sense, and it would be great to have.
That said, I don't think rn I have the time/motivation to do it, but at the same time I don't want to impose this (seemingly annoying) task on you either. So if you want to table it as just a discussion for something to implement in the future I'm happy with that.

@t-bltg
Copy link
Collaborator Author

t-bltg commented Jun 18, 2022

I started something in https://github.com/t-bltg/Term.jl/tree/cols, but it requires a lot more thinking to support Crayons.

What I really like about Crayons is that parsing colors is type dependent, which is very julian and neat IMO:

I don't know if we can integrate this in Term.jl easily.

@FedeClaudi
Copy link
Owner

I'm not sure either! I'd have to look into crayons more...

If there's an easy way to use it to go from strings to ANSI codes it should be fine. The symbol/number -> color conversion here, although nice, is less relevant for term. Term's markup syntax should be mostly string based so that it can be interpolated in normal text strings with no fusses. You could still recover the type under the hood and use that for the colors conversions, ofc.

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

Successfully merging this pull request may close these issues.

Testing broken (again) on some terminal sizes
3 participants