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 then and then_with for ordering. #37054

Merged
merged 8 commits into from
Nov 2, 2016
Merged

Add then and then_with for ordering. #37054

merged 8 commits into from
Nov 2, 2016

Conversation

rednum
Copy link
Contributor

@rednum rednum commented Oct 9, 2016

Fixes #37053 (see discussion in rust-lang/rfcs#1677).

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@@ -245,6 +245,76 @@ impl Ordering {
Greater => Less,
}
}

/// Chain two orderings.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/Chain/Chains/

}
}

/// Chain the ordering with given function.
Copy link
Contributor

@apasel422 apasel422 Oct 9, 2016

Choose a reason for hiding this comment

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

Chains the ordering with the given function.

@sfackler sfackler added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Oct 10, 2016
@rednum
Copy link
Contributor Author

rednum commented Oct 23, 2016

Fixed doctests, can you please take another look? Should I rename this as discussed in the rust-lang/rfcs#1677 ?

@alexcrichton
Copy link
Member

@rfcbot fcp merge

Seems like a nifty API to me!

@rfcbot
Copy link

rfcbot commented Oct 24, 2016

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

Concerns:

Once these reviewers reach consensus, 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.

@aturon
Copy link
Member

aturon commented Oct 25, 2016

@rfcbot concern Naming

This looks like handy functionality, but I think we should consider the naming a bit. I imagine the proposed name is based on analogy with methods on e.g. Option? What this is implementing is something like "lexicographic ordering" (aka "dictionary ordering"), but I'm not sure whether those terms would be helpful/ergonomic to use.

My basic fear is that, when reading code, or does not immediately suggest the actual semantics here.

Unfortunately, nothing better is leaping to mind. Anybody else share this concern, or have alternative ideas?

@alexcrichton
Copy link
Member

Hm yeah that is true. In retrospect if I initially saw something like Ordering::Less.or(Ordering::Greater) I wouldn't actually know what that would do!

Other thoughts brought up in rust-lang/rfcs#1677 were and/and_then, chain/chain_then.

I sort of like the name chain but agree that the closure-taking variant would sound odd.

@rednum
Copy link
Contributor Author

rednum commented Oct 25, 2016

I've used the name or as analogy to Option, but after giving it a second thought I agree it's not the obvious naming. In Java 8 similar feature is called thenComparing. So I think another possibilty would be to call it then (as also suggested in original RFC) and the functional counterpart then_fn (I think and_then would be a bit confusing, because it already does something else for Option).

@aturon
Copy link
Member

aturon commented Oct 25, 2016

@rednum then indeed sounds better! I'd suggest then_with for the closure variant.

@aturon
Copy link
Member

aturon commented Oct 31, 2016

@rfcbot resolved Naming

@bors
Copy link
Contributor

bors commented Nov 1, 2016

☔ The latest upstream changes (presumably #37299) made this pull request unmergeable. Please resolve the merge conflicts.

@brson
Copy link
Contributor

brson commented Nov 1, 2016

I think the names without precedent are unfortunate, but can't think of anything better.

@rfcbot
Copy link

rfcbot commented Nov 1, 2016

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

psst @alexcrichton, I wasn't able to add the final-comment-period label, please do so.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Nov 1, 2016

📌 Commit 655effe has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Nov 2, 2016

⌛ Testing commit 655effe with merge acfe959...

bors added a commit that referenced this pull request Nov 2, 2016
Add or and or_else for ordering.

Fixes #37053 (see discussion in rust-lang/rfcs#1677).
@bors bors merged commit 655effe into rust-lang:master Nov 2, 2016
@apasel422 apasel422 mentioned this pull request Nov 19, 2016
@mbrubeck mbrubeck changed the title Add or and or_else for ordering. Add then and then_with for ordering. Sep 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

9 participants