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

Add resetted styling #40

Merged
merged 6 commits into from
May 24, 2023
Merged

Add resetted styling #40

merged 6 commits into from
May 24, 2023

Conversation

alexkunde
Copy link

Hi,
gnu coreutils are are in parts resetting styles everytime they are applied.
To make this available in uucoreutils (the rust drop in replacement for gnu core utils) I added a chainable style called "resetted" which will print the reset style before the actual string.

Related PR

    test!(bold:                  Style::new().bold();               "hi" => "\x1B[1mhi\x1B[0m");
    test!(bold_with_reset:       Style::new().resetted().bold();    "hi" => "\x1B[0m\x1B[1mhi\x1B[0m");
    test!(bold_with_reset_2:     Style::new().bold().resetted();    "hi" => "\x1B[0m\x1B[1mhi\x1B[0m");

Please see the code and I hope this feature can make it.

@fdncred
Copy link

fdncred commented May 22, 2023

Thanks. In general I don't have issue with what you're trying to do there but I'm not sure I like the resetted term. I'm up for discussion alternatives, but I'm leaning towards something that says what it does like reset_before_style

@alexkunde
Copy link
Author

Hi, thank you for considering and your feedback! Whatever naming feels best for you, I will go with. Should I rename to your proposal?

@fdncred
Copy link

fdncred commented May 23, 2023

Should I rename to your proposal?

If you don't have a better suggestion, then yes. We appreciate your help!

@alexkunde
Copy link
Author

Allright, I will have a look once the other PR is merged, since itll need a refactor of tests anyway than.

@alexkunde
Copy link
Author

Hi, I've renamed the function and tests are green.

@alexkunde
Copy link
Author

I will also do a quick update of the readme

@alexkunde
Copy link
Author

done

@fdncred
Copy link

fdncred commented May 24, 2023

Thanks

@fdncred fdncred merged commit 135430b into nushell:main May 24, 2023
@fdncred
Copy link

fdncred commented May 25, 2023

@schoki040 I'd like to test this in nushell but I can't because lscolors isn't updated. Have you already submitted a PR there?

This is the error I'm getting.

error[E0063]: missing field `with_reset` in initializer of `nu_ansi_term::Style`
   --> /Users/fdncred/.cargo/registry/src/github.com-1ecc6299db9ec823/lscolors-0.14.0/src/style.rs:404:9
    |
404 |         nu_ansi_term::Style {
    |         ^^^^^^^^^^^^^^^^^^^ missing `with_reset`

UPDATE I found this sharkdp/lscolors#66

@alexkunde
Copy link
Author

Hi, yes exactly. Right now im working on getting the pipelines green. Mutually exclusive features and --all-features are not implemented by rust. On the other hand, I used that branch to locally test with LS from coreutils and the gnu tests seems to pass now.

sholderbach added a commit to sholderbach/nu-ansi-term that referenced this pull request Jul 18, 2023
Should include the more friendly API breakage from nushell#47
Still a breaking change compared to 0.47 based on nushell#40
sholderbach added a commit that referenced this pull request Jul 22, 2023
Should include the more friendly API breakage from #47
Still a breaking change compared to 0.47 based on #40
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.

2 participants