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 Style.with_reset more explicit as prefix_with_reset #50

Merged
merged 1 commit into from
Jul 22, 2023

Conversation

sholderbach
Copy link
Member

As the behavior is probably a bit unclear and everybody constructing or
destructuring it by hand will now have to take this field into account
we should be as explicit as possible. @fdncred 's suggestion was
prefix_with_reset.
I also tried to be even more detailed in the fields docstring for
anybody interacting it after 0.47.0. @alexkunde check me if I am wrong
here.

As the behavior is probably a bit unclear and everybody constructing or
destructuring it by hand will now have to take this field into account
we should be as explicit as possible. @fdncred 's suggestion was
`prefix_with_reset`.
I also tried to be even more detailed in the fields docstring for
anybody interacting it after `0.47.0`. @alexkunde check me if I am wrong
here.
Copy link

@fdncred fdncred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I think it's clearer now.

@alexkunde
Copy link

alexkunde commented Jul 21, 2023

hi @sholderbach yes, if you use the struct without ..default() you will run into a breaking change.
On another note, since we are opening reset up again, on working further with LS_Colors is, gnu LS supports a re=xxx which styles the reset. This would be the next coming topic.
what's your opinion, should we tackle it now? or later?

@fdncred
Copy link

fdncred commented Jul 21, 2023

I'd vote for later. We're about to make a nushell release on the 25th and I'd love for it to have the latest nu-ansi-term. That doesn't leave a lot of time to code and test. If it's trivial then it's no big deal though.

@alexkunde
Copy link

Looking at the timeline, I would than also vote for later, as I dont really have time this weekend to look into the matter. Even without reset colors this PR is a very good step forward!

@sholderbach sholderbach merged commit b972a62 into nushell:main Jul 22, 2023
2 checks passed
@sholderbach sholderbach deleted the more-explicit-style-reset branch July 22, 2023 15:43
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.

None yet

3 participants