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

Allow dereferencing AnsiString as underlying string #12

Merged
merged 1 commit into from
Oct 6, 2022

Conversation

richardhozak
Copy link

@richardhozak richardhozak commented Oct 5, 2022

Hello, I've been in process of converting https://github.com/ogham/exa from rust-ansi-term to nu-ansi-term and there is one api that exa needs that was lost during the refactoring from rust-ansi-term to nu-ansi-term.

exa needs the underlying raw string to calculate widths, previously this api was present in a form, where AnsiString could be dereferenced into Cow<'a, str>, directly and the to str, which was then used.

This adds the minimal required code, it just returns &str which is the minimum needed.

I tried to workaround this with some other api but could not found any in nu-ansi-term and to_string would require allocation which seemed unnecessary as the underlying view can be returned directly.

@sholderbach
Copy link
Member

I removed the Deref implementation in 1df156e because we were running into issues with clippy suggesting to remove to_string() calls that were actually necessary to apply the styling.
Sadly I wasn't able to cook up a working minimal reproducible example to check with the clippy folks if that is a false positive or if the original implementation was unsound.

@richardhozak
Copy link
Author

I see, but this should not trigger the lint I think, the problem seems to be that it was implemented as Deref, this is just ordinary getter.

Copy link
Member

@sholderbach sholderbach left a comment

Choose a reason for hiding this comment

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

Yeah your PR looks good to me and handles as expected in playing with it. I just wanted to keep the historical record around, for those coming from the original ansi_term crate

@sholderbach sholderbach merged commit 67eb4e4 into nushell:main Oct 6, 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

Successfully merging this pull request may close these issues.

2 participants