Tracking Issue for `Result::into_ok_or_err` / `feature(result_into_ok_or_err)` ·...
source link: https://github.com/rust-lang/rust/issues/82223
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
Tracking Issue for Result::into_ok_or_err
/ feature(result_into_ok_or_err)
#82223
1 of 5 tasks
thomcc opened this issue on Feb 17, 2021 · 36 comments
Comments
Feature gate: This is a tracking issue for Public API
Steps / History
Unresolved Questions
|
added C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
labels
I don't feel that strongly, but considered this (it was suggested to me when implementing the PR) and didn't do it because it can be achieved using The same is true for a hypothetical |
Contributor
camsteffen commented on Feb 17, 2021
I would narrow down the names to ones that are |
The API surface of Result/Option is quite large already. The bar for adding even more methods for them should be raised because it's getting harder for me to remember and use all of them. |
Member
Author
thomcc commented on Feb 17, 2021
There are multiple cases in the stdlib (e.g. ignoring external crates) where something like this is beneficial. The alternatives are less clear ( There were also multiple PRs proposing this, showing it's not just me that considers it to meet the bar for addition. I also think that broadly the current design of these types (and many other APIs in libcore, Iterator as an example, or the functions on the numeric types) favor functionality over minimalism in the API, but that's more of a philosophical question. You also don't have to use them if you fail to remember them. |
Please don't say this, it's a fallacy :-) |
Contributor
faern commented on Mar 23, 2021
This method became more relevant since deprecating |
Yeah, that actually motivated this. Edit: FWIW in the mean time I think |
Contributor
memoryruins commented on Mar 27, 2021
With
The method this issue tracks could still be useful for chaining, but I thought I may as well mention the above ^^ |
Contributor
TheDan64 commented on May 20, 2021
Calling it |
@rfcbot merge This is
There are a lot of possible names for this function, but I think |
Team member @m-ou-se has proposed to merge this. The next step is review by the rest of the tagged team members: Concerns:
Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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. |
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
I think the name here being a bit verbose is fine, because it's really only for slightly-weird situations, as normally one would have an error as E and a non-error as T, in which case this is method doesn't apply. Thus a slightly-longer name to help emphasize it's in the unusual T == E case makes sense. |
Of the above comments, I think my perspective most closely matches #82223 (comment) and #82223 (comment). The bar for whether a method goes on
So we're starting with a niche set of methods (a few percent of crates ever encounter a Result<T, T>), and a fraction of the call sites of those methods, and not really offering a substantial improvement to even those. From the perspective of looking at how ubiquitously a transformation is needed and how much giving it a name can facilitate understandability of the code, I would prefer
This is not a convincing justification for @rfcbot concern rather not |
I agree with @dtolnay on this one, though I think the second comment he linked is the much more persuasive justification for me. The fact that we already have stable or-patterns that let you write almost the exact same thing, and which is useful in far more situations makes me strongly prefer we focus on pushing users towards using or-patterns rather than stabilizing this API. In general I prefer a smaller API surface area with more expressive and composable features to one with many useful helper methods, since it's much easier to remember a smaller set of features. The only difference I can see between this API and or-patterns is that this API can be used in the expression position, but or-patterns are exclusively used in let statements or match expressions and require an extra subsequent expression to refer to the bound variable. This will probably be pretty annoying if you're forced to break up a long chain of method calls but I would prefer to resolve this by finding a small additional language syntax (some sort of postfix match expressions maybe? As a follow up though I imagine that many users don't know about or-patterns, so I would love to see a PR where we add some additional docs to the relevant APIs in For binary search it looks like we already have a relevant example which we could update.
|
Interesting, I was expecting that number to be higher. We should also check out |
(I should also note that |
Recently, I've read the interface design rules in the "Rust for Rustaceans" book and it mentions "The Principle of Least Surprise". Personally, I dislike using let pattern for that - it looks noisy and less clear. Btw. @m-ou-se thanks for mentioning partition_point, it's indeed more meaningful for this usecase! |
I went ahead and submitted a PR to update the docs for |
Was looking at using this, not for a message returning It is not "Rustic" to use |
Maybe it's time to propose that though? So many crates define their own already ( The standard library contains seemingly-redundant things in other places too, e.g. |
Though it does define https://docs.rs/itertools/latest/itertools/enum.EitherOrBoth.html for |
This comment was marked as disruptive content.
removed 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
Contributor
Stargateur commented on May 31
@jplatte I also think we need a I also believe such feature on Result is error prone for user, the need to be very explicit on the name show it. |
Contributor
Lokathor commented 5 days ago
I wanted to get a reader on an RrLock and also put a type on the binding so that it was clear what was going on, to do that using or-patterns you have to write... this entire thing:
So I don't really think that or-patterns are as good a solution as having a method, because to make them work nicely you end up being nearly forced to not write down as many types in the code. |
Member
dtolnay commented 5 days ago
@Lokathor if I understand correctly, you want to write this?
I think
|
Member
dtolnay commented 5 days ago
@rfcbot fcp close Nothing compelling has been brought forth since then. I would like to remove this method. That's right; I don't understand how this would possibly be controversial. |
rfcbot commented 5 days ago •
Team member @dtolnay has proposed to close this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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. |
added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-close This PR / issue is in PFCP or FCP with a disposition to close it.
labels
Member
dtolnay commented 5 days ago
|
Member
scottmcm commented 5 days ago
to close. To be useful, this needs either an And @Stargateur's comment above (#82223 (comment)) reinforces that for me. The problem here is not the isomorphic structure of the enum, but the semantic meaning of the variants here. A method like this is totally fine on |
Contributor
Lokathor commented 4 days ago
This is certainly not the place to have that extended discussion, which I think you already know, so I don't know why you brought it up. |
I agree we should close this. We have very few cases where we use a For The equivalent of the now deprecated There are not many usages of Similar for |
added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised.
and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off.
labels
rfcbot commented 4 days ago
This is now entering its final comment period, as per the review above. |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
No one assigned
None yet
No milestone
No branches or pull requests
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK