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

str's trim_left and trim_right implicitly assume LTR text #30459

Closed
huonw opened this issue Dec 18, 2015 · 18 comments
Closed

str's trim_left and trim_right implicitly assume LTR text #30459

huonw opened this issue Dec 18, 2015 · 18 comments
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@huonw
Copy link
Member

huonw commented Dec 18, 2015

These functions and their _matches variants operate on the start and end of the string, which are only the left and right (respectively) of the displayed text when rendered left-to-right like English. For (readers/users of) RTL languages, trim_right is hinting that it will be trimming the prefix, but this is of course not the case (and similarly for _left).

These are stable and there is precedent in other languages for left/right e.g. Python (there's also significant precedent for start/end), so we may not be able/want to do anything here, but it seems worth noting.

@huonw huonw added I-nominated T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Dec 18, 2015
@sfackler
Copy link
Member

I seem to recall Android adding start and end variants of some alignment constraints and deprecating the old left and right versions for this reason.

@SimonSapin
Copy link
Contributor

+1 for start and end. CSS introduced the same keywords when generalizing text layout to RTL and vertical text. (And inline-start, inline-end, block-start, block-end when disambiguation is needed.)

How does the libs team feel about deprecating stable APIs?

@abonander
Copy link
Contributor

Can confirm: left/right => start/end on Android. In XML the former pair is only linted against; I don't know if it's deprecated in the Java APIs. It's kind of annoying because I also use left/right for layout alignments that shouldn't change between LTR and RTL.

I don't think we need to deprecate these methods. A documentation tweak could clarify the current implications on their behavior. Do we even have a way to detect an RTL configuration anyways? Perhaps this is best left to an external crate to implement these methods as a precursor to a more general localization framework.

@huonw
Copy link
Member Author

huonw commented Dec 19, 2015

Do we even have a way to detect an RTL configuration anyways?

No we don't.

Perhaps this is best left to an external crate to implement these methods as a precursor to a more general localization framework.

Also, I don't really understand what you mean by an external crate doing this: there is absolutely no intention for these functions to change so to have trim_right == start for RTL text, i.e. trim_left will always operate on the start and trim_right always on the end. Consistently trimming at the start/end no matter the direction of text seems likely to be the desired behaviour in most circumstances (i.e. even ones that encounter non-LTR text).

@SimonSapin
Copy link
Contributor

I don’t think anything in the standard library should behave differently based on text direction. This stuff is more complex than you might think: http://www.unicode.org/reports/tr9/

(If you really want to do proper text layout, it’s a job for https://github.com/servo/unicode-bidi and other crates.)

Left and right in str::trim_left and str::trim_right really mean lower memory addresses and higher memory addresses, so that naming is only correct when the text happens to be left-to-right. I’m proposing to add str::trim_start and str::trim_end with the same behavior, but more correct naming. And maybe deprecating the old names. Similarly with other methods with left and right in the name.

Methods with an r prefix (like rfind) have the same problem, but I don’t have a good replacement to suggest right now.

@abonander
Copy link
Contributor

Okay, I see now. The text gets reversed during rendering--in memory the start of the string is at index 0 regardless of configuration. In that case I don't really have anything against the change itself.

As for the r prefix, I assumed it meant "reverse", which still makes sense in RTL. We can clarify that in documentation.

@SimonSapin
Copy link
Contributor

Yes, http://www.unicode.org/reports/tr9/#Introduction explains that memory representation of Unicode text is in "logical order" and gives some more details.

Ok, rfind as "reverse find" rather than "find from the right" works.

@aturon aturon removed the I-nominated label Jan 8, 2016
@aturon
Copy link
Member

aturon commented Jan 8, 2016

Libs team consensus: for now, we should make sure the documentation is clear about exactly what's happening here. In the long run, we'd be interested in a deprecation/renaming, but we'd like to have some better tools in place to do so.

The problem is that if we just deprecate it normally, all crates using the functionality have to choose between deprecation messages and upping the version of Rust they require. Neither choice is great.

The core team has discussed the idea of regular "long term support" releases, or otherwise some way to flag releases in a coarser-grained way. That can be useful for deprecations like this -- they would give something more like a small "epoch of Rust" to attach things like deprecations to, rather than forcing lib authors to just arbitrarily require Rust v1.7+ or whatever.

More broadly, we'd like to establish a policy for this kind of minor cleanup -- where the change makes things more clear, but doesn't close any fundamental holes or anything.

@SimonSapin
Copy link
Contributor

This is a very valid concern. Has a more fine-grained #[allow(deprecated(some feature))] been considered?

@aturon
Copy link
Member

aturon commented Jan 8, 2016

@SimonSapin yes, that idea has come up from time to time, and would definitely be helpful here.

The main drawback is that it loses some of the point of deprecation -- if it's easy to opt out of the deprecation, there's little incentive to actually improve your code.

Of course, in this particular case the use of deprecation is more about discoverability/documentation/API clarity than improving existing code per se.

@SimonSapin
Copy link
Contributor

Other random idea: add the new name and deprecate the new one in a way that’s visible in the documentation but doesn’t emit a warning yet. (At least until the new name hits the stable channel.)

@aturon
Copy link
Member

aturon commented Jan 8, 2016

@SimonSapin Yes, that's a feature we absolutely want -- basically, the ability to say "deprecated since Rust 1.8" or some other future release, and have the compiler/rustdoc do the right things.

I'll open an issue for it.

@aturon
Copy link
Member

aturon commented Jan 8, 2016

cc #30785

@sfackler
Copy link
Member

sfackler commented Jan 8, 2016

A possible alternative to #[allow(deprecated(some feature))] is something like #[allow(deprecated(after = "1.3.0"))], i.e. you saying that you explicitly are supporting back to 1.3, so deprecations made after that point are probably not relevant to you.

@aturon
Copy link
Member

aturon commented Jan 8, 2016

Oh, and another idea we've talked about: the ability to cfg on specific feature stabilizations or perhaps Rust versions. That makes it more feasible to straddle deprecations.

steveklabnik added a commit to steveklabnik/rust that referenced this issue Jan 26, 2016
@steveklabnik
Copy link
Member

Removing docs since my PR is in, now just a libs question if we want new versions.

steveklabnik added a commit to steveklabnik/rust that referenced this issue Feb 1, 2016
Fixes rust-lang#30459

Fun fact: i wanted to write "Arabic" and "Hebrew" in Arabic and Hebrew, but vim kept doing the copy/paste in the wrong direction.
steveklabnik added a commit to steveklabnik/rust that referenced this issue Feb 2, 2016
Fixes rust-lang#30459

Fun fact: i wanted to write "Arabic" and "Hebrew" in Arabic and Hebrew, but vim kept doing the copy/paste in the wrong direction.
@huonw
Copy link
Member Author

huonw commented May 27, 2016

I believe #31202 closed this accidentally. At least, I can't see any documented decision from the libs team.

@huonw huonw reopened this May 27, 2016
@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 24, 2017
kennytm added a commit to kennytm/rust that referenced this issue Apr 3, 2018
…isdreavus,GuillaumeGomez

Handle future deprecation annotations

This adds special handling to the `since` parameter of the `deprecated` attribute: in particular, if the `since` version exceeds the version of the compiler, the deprecation notice will not be printed; but a note is added to the documentation stating that the item will be deprecated in a later version.

(I've used `since` for this, rather than adding a new attribute, because it's more seamless and, I feel, intuitive. Plus it involves less code churn.)

![image](https://user-images.githubusercontent.com/3943692/37611317-ef5cdf16-2b99-11e8-8251-e35e8f7b0137.png)
![image](https://user-images.githubusercontent.com/3943692/37611323-f748c2d0-2b99-11e8-966b-11408c73d416.png)

This is a prerequisite for doing things renaming methods in the standard library (e.g. rust-lang#30459). Resolves rust-lang#30785.
kennytm added a commit to kennytm/rust that referenced this issue Apr 4, 2018
…isdreavus,GuillaumeGomez

Handle future deprecation annotations

This adds special handling to the `since` parameter of the `deprecated` attribute: in particular, if the `since` version exceeds the version of the compiler, the deprecation notice will not be printed; but a note is added to the documentation stating that the item will be deprecated in a later version.

(I've used `since` for this, rather than adding a new attribute, because it's more seamless and, I feel, intuitive. Plus it involves less code churn.)

![image](https://user-images.githubusercontent.com/3943692/37611317-ef5cdf16-2b99-11e8-8251-e35e8f7b0137.png)
![image](https://user-images.githubusercontent.com/3943692/37611323-f748c2d0-2b99-11e8-966b-11408c73d416.png)

This is a prerequisite for doing things renaming methods in the standard library (e.g. rust-lang#30459). Resolves rust-lang#30785.
@varkor
Copy link
Member

varkor commented Apr 10, 2018

Now that #30785 is merged, it would be feasible to add unstable str::trim_start and str::trim_end and add a future deprecation for str::trim_left and str::trim_right.

cc @rust-lang/libs

bors added a commit that referenced this issue Jun 22, 2018
…petrochenkov

Support future deprecation for rustc_deprecated

Follow-up to #49179 to allow `since` parameters to be set to future versions of Rust and correspondingly to not be treated as deprecated until that version. This is required for #30459 to be completed (though we'll need to wait until this hits beta).
bors added a commit that referenced this issue Sep 5, 2018
Add trim_start, trim_end etc.; deprecate trim_left, trim_right, etc. in future

Adds the methods: `trim_start`, `trim_end`, `trim_start_matches` and `trim_end_matches`.
Deprecates `trim_left`, `trim_right`, `trim_left_matches` and `trim_right_matches` starting from Rust 1.33.0, three versions from when they'll initially be marked as being deprecated, using the future deprecation from #30785 and #51681.

Fixes #30459.
sporksmith added a commit to sporksmith/feedburst that referenced this issue Apr 19, 2020
Rename to trim_start and trim_end for consistency with String, and in
the spirit of their rename (not assuming RTL):
rust-lang/rust#30459
sunfishcode added a commit to sunfishcode/rust that referenced this issue May 19, 2022
…:rfind`.

In the documentation comment for `std::str::rfind`, say "last" instead
of "rightmost" to describe the match that `rfind` finds. This follows the
spirit of rust-lang#30459, for which `trim_left` and `trim_right` were replaced by
`trim_start` and `trim_end` to be more clear about how they work on
text which is displayed right-to-left.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue May 20, 2022
…r=thomcc

Say "last" instead of "rightmost" in the documentation for `std::str:rfind`

In the documentation comment for `std::str::rfind`, say "last" instead
of "rightmost" to describe the match that `rfind` finds. This follows the
spirit of rust-lang#30459, for which `trim_left` and `trim_right` were replaced by
`trim_start` and `trim_end` to be more clear about how they work on
text which is displayed right-to-left.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants