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

Implement Display for CellPath #11023

Merged
merged 1 commit into from Nov 10, 2023
Merged

Conversation

IanManske
Copy link
Member

Description

Because CellPath::into_string takes a borrowed self, I renamed it to to_string to follow Rust API guidelines. This then triggered the clippy lint inherent_to_string, which is... correct! The current CellPath::into_string is being used as if it were the Display implementation for CellPath.

User-Facing Changes

Breaking API change for nu-protocol, since CellPath::into_string was removed.

Copy link
Member

@amtoine amtoine left a comment

Choose a reason for hiding this comment

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

sounds fair to me 😋 🙏

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.

Good call! In theory this should make some display operations of cell paths cheaper as it can now write instead of allocating a small stirng.

@sholderbach sholderbach merged commit 93096a0 into nushell:main Nov 10, 2023
19 checks passed
gaetschwartz pushed a commit to gaetschwartz/nushell that referenced this pull request Nov 11, 2023
# Description
Because `CellPath::into_string` takes a borrowed `self`, I renamed it to
`to_string` to follow Rust [API
guidelines](https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv).
This then triggered the clippy lint
[inherent_to_string](https://rust-lang.github.io/rust-clippy/master/index.html#/inherent_to_string),
which is... correct! The current `CellPath::into_string` is being used
as if it were the `Display` implementation for `CellPath`.

# User-Facing Changes
Breaking API change for `nu-protocol`, since `CellPath::into_string` was
removed.
@IanManske IanManske deleted the cellpath-display branch November 15, 2023 16:40
hardfau1t pushed a commit to hardfau1t/nushell that referenced this pull request Dec 14, 2023
# Description
Because `CellPath::into_string` takes a borrowed `self`, I renamed it to
`to_string` to follow Rust [API
guidelines](https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv).
This then triggered the clippy lint
[inherent_to_string](https://rust-lang.github.io/rust-clippy/master/index.html#/inherent_to_string),
which is... correct! The current `CellPath::into_string` is being used
as if it were the `Display` implementation for `CellPath`.

# User-Facing Changes
Breaking API change for `nu-protocol`, since `CellPath::into_string` was
removed.
dmatos2012 pushed a commit to dmatos2012/nushell that referenced this pull request Feb 20, 2024
# Description
Because `CellPath::into_string` takes a borrowed `self`, I renamed it to
`to_string` to follow Rust [API
guidelines](https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv).
This then triggered the clippy lint
[inherent_to_string](https://rust-lang.github.io/rust-clippy/master/index.html#/inherent_to_string),
which is... correct! The current `CellPath::into_string` is being used
as if it were the `Display` implementation for `CellPath`.

# User-Facing Changes
Breaking API change for `nu-protocol`, since `CellPath::into_string` was
removed.
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