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 trim_start, trim_end etc.; deprecate trim_left, trim_right, etc. in future #52994

Merged
merged 4 commits into from
Sep 6, 2018

Conversation

varkor
Copy link
Member

@varkor varkor commented Aug 2, 2018

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.

@rust-highfive
Copy link
Collaborator

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 2, 2018
@cramertj
Copy link
Member

cramertj commented Aug 2, 2018

Assigning someone on libs... r? @dtolnay

@rust-highfive rust-highfive assigned dtolnay and unassigned cramertj Aug 2, 2018
@rust-highfive

This comment has been minimized.

@kennytm kennytm added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 3, 2018
/// assert!(Some('E') == s.trim_start().chars().next());
///
/// let s = " עברית";
/// assert!(Some('ע') == s.trim_start().chars().next());
Copy link
Member

Choose a reason for hiding this comment

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

On my computer this example shows up like this:

selection_110

The whitespace at the low byte positions of the string appear on the left, and the right-to-left characters in the high byte positions appear on the right. The documentation explains that trim_start trims the right side of right-to-left text, and in fact trimming is taking place, but as rendered it appears that no trimming has happened. Is this something that would be clear to people who regularly deal with right-to-left? Is there a way to make an example that is less ambiguous for people not experienced with right-to-left?

Copy link
Member Author

@varkor varkor Aug 5, 2018

Choose a reason for hiding this comment

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

That's a good point. Examples are tricky with mixed text directions. I'll try to come up with a better example.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added whitespace to both sides. It should be clear to anyone that the leading whitespace is being trimmed, even if they don't realise which spaces are actually being removed. Anything more subtle requires actually explaining how mixed directional text is actually rendered, which we probably don't want to do here.

/// let s = " עברית";
/// assert!(Some('ע') == s.trim_start().chars().next());
/// ```
#[unstable(feature = "trim_direction", issue = "30459")]
Copy link
Member

Choose a reason for hiding this comment

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

I believe we will want to land these methods as stable straight away. The #30785 future deprecation doesn't help if it takes us another 3 cycles to stabilize the replacement -- the idea was to leave 3 cycles of transition time during which both new and old are usable without warnings.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, of course!

@@ -25,6 +25,7 @@
#![feature(unboxed_closures)]
#![feature(exact_chunks)]
#![feature(repeat_generic_slice)]
#![feature(trim_direction)]
Copy link
Member

Choose a reason for hiding this comment

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

This can be removed here and in the doc tests if we are landing these as stable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops, fixed.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think the fix has been pushed yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just noticed I pushed the wrong branch. It's definitely fixed now 😅

@dtolnay

This comment has been minimized.

@kennytm

This comment has been minimized.

@dtolnay
Copy link
Member

dtolnay commented Aug 6, 2018

@rust-lang/libs this PR adds stable methods intended as direct replacements for the existing str::trim_left, trim_right, trim_left_matches, trim_right_matches.

impl str {
    pub fn trim_start(&self) -> &str;

    pub fn trim_end(&self) -> &str;

    pub fn trim_start_matches<'a, P>(&'a self, pat: P) -> &'a str
    where
        P: Pattern<'a>;

    pub fn trim_end_matches<'a, P>(&'a self, pat: P) -> &'a str
    where
        P: Pattern<'a>,
        P::Searcher: ReverseSearcher<'a>;
}

The signatures are the same but the new names better describe the behavior on right-to-left input text. The old names will display a deprecation notice in the documentation but are not set to warn until 1.33.0.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Aug 6, 2018

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Aug 6, 2018
@withoutboats
Copy link
Contributor

withoutboats commented Aug 8, 2018

@rfcbot concern deprecation-schedule

I'm a big +1 on adding the more accurately named methods, but I'm uncertain that deprecating the old methods in 1.33 is the best decision. I don't see any discussion on that choice on this or the issue it closes. I'm not against it, I just want to raise it for discussion and see if anyone has anything to say about it before this goes into FCP.

I've checked my box so once I resolve this concern the PR will go into FCP.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Aug 8, 2018
@BurntSushi
Copy link
Member

@withoutboats I have a vague unstructured opinion that we're a bit too quick to deprecate things in general, and I would probably apply it here too. But it's honestly a pretty weak opinion, particularly if we do add the new methods anyway. Here's a trichotomy to make my view a little clearer:

  1. Do nothing. Keep current methods; don't add new methods.
  2. Add new methods. Keep current methods and don't deprecate them.
  3. Add new methods. Keep current methods and deprecate them.

On a small scale, I'd probably go with (3) (because clarifications like this are nice). On a larger scale, I'd probably go with (1) (because there's churn, and I think there is a lot more room to accept warts). Route (2) seems somewhat inconsistent with what we've done in the past. That is, I don't think we have a lot of "this thing is an exact alias to this other thing." So in that sense, if we definitely want these new methods, I'd probably just go ahead any deprecate the existing ones.

What thoughts did you have on this?

@varkor
Copy link
Member Author

varkor commented Aug 8, 2018

I don't see any discussion on that choice on this or the issue it closes.

👍I picked "3 releases from now" fairly arbitrarily, as a period that seemed somewhat reasonable. I probably should have clarified that — the FCP seems like as good a time as any to discuss it, though!

@withoutboats
Copy link
Contributor

withoutboats commented Aug 8, 2018

What thoughts did you have on this?

I really don't know. I guess I feel like these methods are widely used, and so deprecating them will be disruptive. So I'd like some headway before we start issuing warnings. As we can see, @varkor's already done at least some of that by implementing future deprecations & targeting 1.33 for deprecation, but is that enough? I'm not sure.

Some vague questions/thoughts:

  • How will we communicate about this future deprecation prior to 1.33 to try to encourage people to switch over (at least for new code)?
  • How do the future deprecated methods appear in docs? Does it successfully encourage people to switch? (I already asked varkor this on a separate thread) EDIT: Screenshot in Handle future deprecation annotations  #49179
  • What other ways could we encourage people to switch before we start issuing warnings?

I also have no idea how to judge if 3 releases is the correct number.

@SimonSapin
Copy link
Contributor

Considering that:

  • Keeping a code base warning-free is generally nice / desired.
  • The same code can often be compiled with a range of different Rust versions. For example a library supports the Stable channel, but when contributing to it I might use Nightly to take advantage of tooling (not language or libs) improvements.

I’ve generally argued for not emitting deprecation warnings in Nightly until the recommended replacement is available on stable. So that means a 2 release cycles delay. However that’s a minimum, increasing it to 3 doesn’t cost us much and makes things easier for projects that mostly track Stable but still pin to a specific version, leaving them some more time to upgrade. For example Firefox’s policy is to upgrade 2 weeks after a new Stable is released (modulo blocking bugs).

For projects that have a policy of keeping compat with a specific version older than Stable, they’ll likely not want to bump their requirement for a simple renaming like this and might use #[allow(deprecated)] instead. So I’d be fine with a longer delay before we start emitting warnings.

Does rustdoc show an API as deprecated even before we start emitting warnings based on since? If not I think it should.

@withoutboats
Copy link
Contributor

Does rustdoc show an API as deprecated even before we start emitting warnings based on since? If not I think it should.

Yea, you can see a screenshot in #49179

@withoutboats
Copy link
Contributor

withoutboats commented Aug 29, 2018

@rfcbot resolve deprecation-schedule

Seems like there's no further progress to be made from discussing the deprecation schedule. Here's my solution:

  1. I've opened an issue to discuss automating rename deprecations by adding a field to the attribute: Support cargo fix for rename deprecations #53802
  2. We should make sure to include in the release notes all future deprecations, so that people will see them (that is, these will be in the release notes 3 times before the deprecation actually happens).

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Aug 29, 2018
@rfcbot
Copy link

rfcbot commented Aug 29, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Aug 29, 2018
bors added a commit that referenced this pull request Sep 1, 2018
Add Error::source method per RFC 2504.

This implements part of RFC 2504.

* Adds `Error::source`, a replacement for `Error::cause` with the "right" signature, which will be instantly stable.
* Deprecates `Error::cause` in 1.33 (this choice was based on the precedent in #52994, which we haven't finalized).
* Redefines `Error::cause` to delegate to `Error::source` (the delegation can only go in this direction, not the other).

@rfcbot fcp merge
@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Sep 5, 2018

📌 Commit ff79670 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Sep 5, 2018
@bors
Copy link
Contributor

bors commented Sep 5, 2018

⌛ Testing commit ff79670 with merge 8b7f164...

bors added a commit that referenced this pull request 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.
@bors
Copy link
Contributor

bors commented Sep 6, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 8b7f164 to master...

@bors bors merged commit ff79670 into rust-lang:master Sep 6, 2018
@dtolnay dtolnay added the relnotes Marks issues that should be documented in the release notes of the next release. label Sep 6, 2018
@dtolnay
Copy link
Member

dtolnay commented Sep 6, 2018

Tagging relnotes -- this a future deprecation in 1.33.0 so it will be noted in the next three release notes.

@varkor varkor deleted the trim_direction branch September 6, 2018 13:45
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Sep 8, 2018
flip1995 added a commit to flip1995/rust-clippy that referenced this pull request Dec 14, 2018
s/trim_left/trim_start/

s/trim_right/trim_end/
bors added a commit to rust-lang/rust-clippy that referenced this pull request Dec 14, 2018
rustup rust-lang/rust#52994

`trim_left*` and `trim_right*` are deprecated as of 1.33.0.

`s/trim_left/trim_start/`
`s/trim_right/trim_end/`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 15, 2018
Changes:

````
rustup rust-lang#52994
Fix test
Line length fix
Remove references to sized for end users
Remove DUMMY_SP
Add suggestion for replacement
Update lint definitions
Lint for Vec<Box<T: Sized>> - Closes rust-lang#3530
Fix doc_markdown mixed case false positive
question_mark: Suggest Some(opt?) for if-else
redundant_field_names: Do not trigger on path with type params
question_mark: Lint only early returns
question_mark: Fix applicability
Remove obsolete comment
new_without_default, partialeq_ne_impl: Use span_lint_node
Update .stderr after rebase
cargo fmt and remove stabilized feature
Make suggestion Applicability::MachineApplicable
Address review feedback
Extract method
Check array lengths to prevent OOB access
Add suggestion for explicit_write lint
Fix write_with_newline escaping false positive
````
bors added a commit that referenced this pull request Dec 17, 2018
submodules: update clippy from b7a431e to a416c5e

Changes:

````
rustup #52994
Fix test
Line length fix
Remove references to sized for end users
Remove DUMMY_SP
Add suggestion for replacement
Update lint definitions
Lint for Vec<Box<T: Sized>> - Closes #3530
Fix doc_markdown mixed case false positive
question_mark: Suggest Some(opt?) for if-else
redundant_field_names: Do not trigger on path with type params
question_mark: Lint only early returns
question_mark: Fix applicability
Remove obsolete comment
new_without_default, partialeq_ne_impl: Use span_lint_node
Update .stderr after rebase
cargo fmt and remove stabilized feature
Make suggestion Applicability::MachineApplicable
Address review feedback
Extract method
Check array lengths to prevent OOB access
Add suggestion for explicit_write lint
Fix write_with_newline escaping false positive
````

make toolstate green again
hug-dev added a commit to hug-dev/cc-rs that referenced this pull request Dec 18, 2018
The trim_right_matches method is deprecating in 1.33 in favor of
trim_end_matches (see rust-lang/rust#52994).
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request May 5, 2020
Changes:

````
rustup rust-lang/rust#52994
Fix test
Line length fix
Remove references to sized for end users
Remove DUMMY_SP
Add suggestion for replacement
Update lint definitions
Lint for Vec<Box<T: Sized>> - Closes rust-lang#3530
Fix doc_markdown mixed case false positive
question_mark: Suggest Some(opt?) for if-else
redundant_field_names: Do not trigger on path with type params
question_mark: Lint only early returns
question_mark: Fix applicability
Remove obsolete comment
new_without_default, partialeq_ne_impl: Use span_lint_node
Update .stderr after rebase
cargo fmt and remove stabilized feature
Make suggestion Applicability::MachineApplicable
Address review feedback
Extract method
Check array lengths to prevent OOB access
Add suggestion for explicit_write lint
Fix write_with_newline escaping false positive
````
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.