Alternative Updated std::Option, std::Either and std::Result #8288
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Made naming schemes consistent between Option, Result and Either - Changed Options Add implementation to work like the maybe monad (return None if any of the inputs is None) - Removed duplicate Option::get and renamed all related functions to use the term `unwrap` instead
|
I much prefer |
|
Hmm, well, I see that I'm in the minority here. I guess we should discuss in the meeting. |
|
@pcwalton There have been a lot of proposals here. I took this one as the most conservative stopgap to unify get/unwap until the larger issue of how the box and container methods should be designed is resolved. |
bors
added a commit
that referenced
this pull request
Aug 5, 2013
This is an alternative version to #8268, where instead of transitioning to `get()` completely, I transitioned to `unwrap()` completely. My reasoning for also opening this PR is that having two different functions with identical behavior on a common datatype is bad for consistency and confusing for users, and should be solved as soon as possible. The fact that apparently half the code uses `get()`, and the other half `unwrap()` only makes it worse. If the final naming decision ends up different, there needs to be a big renaming anyway, but until then it should at least be consistent. --- - Made naming schemes consistent between Option, Result and Either - Lifted the quality of the either and result module to that of option - Changed Options Add implementation to work like the maybe Monad (return None if any of the inputs is None) See #6002, especially my last comment. - Removed duplicate Option::get and renamed all related functions to use the term `unwrap` instead See also #7887. Todo: Adding testcases for all function in the three modules. Even without the few functions I added, the coverage wasn't complete to begin with. But I'd rather do that as a follow up PR, I've touched to much code here already, need to go through them again later.
bors
added a commit
that referenced
this pull request
Aug 7, 2013
According to #7887, we've decided to use the syntax of `fn map<U>(f: &fn(&T) -> U) -> U`, which passes a reference to the closure, and to `fn map_move<U>(f: &fn(T) -> U) -> U` which moves the value into the closure. This PR adds these `.map_move()` functions to `Option` and `Result`. In addition, it has these other minor features: * Replaces a couple uses of `option.get()`, `result.get()`, and `result.get_err()` with `option.unwrap()`, `result.unwrap()`, and `result.unwrap_err()`. (See #8268 and #8288 for a more thorough adaptation of this functionality. * Removes `option.take_map()` and `option.take_map_default()`. These two functions can be easily written as `.take().map_move(...)`. * Adds a better error message to `result.unwrap()` and `result.unwrap_err()`.
Dylan-DPC
added a commit
to Dylan-DPC/rust
that referenced
this pull request
Apr 7, 2022
Mailmap update I noticed there are a lot of contributors who appear multiple times in https://thanks.rust-lang.org/rust/all-time/, which makes their "rank" on that page inaccurate. For example Nick Cameron currently appears at rank 21 with 2010 contributions and at rank 27 with 1287 contributions, because some of those are from nrc&rust-lang#8288;`@ncameron.org` and some from ncameron&rust-lang#8288;`@mozilla.com.` In reality Nick's rank would be 11 if counted correctly, which is a large difference. Solving this in a totally automated way is tricky because it involves figuring out whether Nick is 1 person with multiple emails, or is 2 people sharing the same name. This PR addresses a subset of the cases: only where a person has committed under multiple names using the same email. This is still not something that can be totally automated (e.g. by modifying https://github.com/rust-lang/thanks to dedup by email instead of name+email) because: - Some emails are not necessarily unique to one contributor, such as `ubuntu@localhost`. - It involves some judgement and mindfulness in picking the "canonical name" among the names used with a particular email. This is the name that will appear on thanks.rust-lang.org. Humans change their names sometimes and can be sensitive or picky about the use of names that are no longer preferred. For the purpose of this PR, I've tried to stick to the following heuristics which should be unobjectionable: - If one of the names is currently set as the display name on the contributor's GitHub profile, prefer that name. - If one of the names is used exclusively over the others in chronologically newer pull requests, prefer the newest name. - If one of the names has whitespace and the other doesn't (i.e. is username-like), such as `Foo Bar` vs `FooBar` or `foobar` or `foo-bar123`, but otherwise closely resemble one another, then prefer the human-like name. - If none of the above suffice in determining a canonical name and the contributor has some other name set on their GitHub profile, use the name from the GitHub profile. - If no name on their GitHub profile but the profile links to their personal website which unambiguously identifies their preferred name, then use that name. I'm also thinking about how to handle cases like Nick's, but that will be a project for a different PR. Basically I'd like to be able to find cases of the same person making commits that differ in name *and* email by looking at all the commits present in pull requests opened by the same GitHub user. <details> <summary>script</summary> ```toml [dependencies] anyhow = "1.0" git2 = "0.14" mailmap = "0.1" ``` ```rust use anyhow::{bail, Context, Result}; use git2::{Commit, Oid, Repository}; use mailmap::{Author, Mailmap}; use std::collections::{BTreeMap as Map, BTreeSet as Set}; use std::fmt::{self, Debug}; use std::fs; use std::path::Path; const REPO: &str = "/git/rust"; fn main() -> Result<()> { let repo = Repository::open(REPO)?; let head_oid = repo .head()? .target() .context("expected head to be a direct reference")?; let head = repo.find_commit(head_oid)?; let mailmap_path = Path::new(REPO).join(".mailmap"); let mailmap_contents = fs::read_to_string(mailmap_path)?; let mailmap = match Mailmap::from_string(mailmap_contents) { Ok(mailmap) => mailmap, Err(box_error) => bail!("{}", box_error), }; let mut history = Set::new(); let mut merges = Vec::new(); let mut authors = Set::new(); let mut emails = Map::new(); let mut all_authors = Set::new(); traverse_left(head, &mut history, &mut merges, &mut authors, &mailmap)?; while let Some((commit, i)) = merges.pop() { let right = commit.parents().nth(i).unwrap(); authors.clear(); traverse_left(right, &mut history, &mut merges, &mut authors, &mailmap)?; for author in &authors { all_authors.insert(author.clone()); if !author.email.is_empty() { emails .entry(author.email.clone()) .or_insert_with(Map::new) .entry(author.name.clone()) .or_insert_with(Set::new); } } if let Some(summary) = commit.summary() { if let Some(pr) = parse_summary(summary)? { for author in &authors { if !author.email.is_empty() { emails .get_mut(&author.email) .unwrap() .get_mut(&author.name) .unwrap() .insert(pr); } } } } } for (email, names) in emails { if names.len() > 1 { println!("<{}>", email); for (name, prs) in names { let prs = DebugSet(prs.iter().rev()); println!(" {} {:?}", name, prs); } } } eprintln!("{} commits", history.len()); eprintln!("{} authors", all_authors.len()); Ok(()) } fn traverse_left<'repo>( mut commit: Commit<'repo>, history: &mut Set<Oid>, merges: &mut Vec<(Commit<'repo>, usize)>, authors: &mut Set<Author>, mailmap: &Mailmap, ) -> Result<()> { loop { let oid = commit.id(); if !history.insert(oid) { return Ok(()); } let author = author(mailmap, &commit); let is_bors = author.name == "bors" && author.email == "[email protected]"; if !is_bors { authors.insert(author); } let mut parents = commit.parents(); let parent = match parents.next() { Some(parent) => parent, None => return Ok(()), }; for i in 1..1 + parents.len() { merges.push((commit.clone(), i)); } commit = parent; } } fn parse_summary(summary: &str) -> Result<Option<PullRequest>> { let mut rest = None; for prefix in [ "Auto merge of #", "Merge pull request #", " Manual merge of #", "auto merge of #", "auto merge of pull req #", "rollup merge of #", "Rollup merge of #", "Rollup merge of #", "Rollup merge of ", "Merge PR #", "Merge #", "Merged #", ] { if summary.starts_with(prefix) { rest = Some(&summary[prefix.len()..]); break; } } let rest = match rest { Some(rest) => rest, None => return Ok(None), }; let end = rest.find([' ', ':']).unwrap_or(rest.len()); let number = match rest[..end].parse::<u32>() { Ok(number) => number, Err(err) => { eprintln!("{}", summary); bail!(err); } }; Ok(Some(PullRequest(number))) } fn author(mailmap: &Mailmap, commit: &Commit) -> Author { let signature = commit.author(); let name = String::from_utf8_lossy(signature.name_bytes()).into_owned(); let email = String::from_utf8_lossy(signature.email_bytes()).into_owned(); mailmap.canonicalize(&Author { name, email }) } #[derive(Copy, Clone, Ord, PartialOrd, Eq, PartialEq)] struct PullRequest(u32); impl Debug for PullRequest { fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result { write!(formatter, "#{}", self.0) } } struct DebugSet<T>(T); impl<T> Debug for DebugSet<T> where T: Iterator + Clone, T::Item: Debug, { fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result { formatter.debug_set().entries(self.0.clone()).finish() } } ``` </details>
Dylan-DPC
added a commit
to Dylan-DPC/rust
that referenced
this pull request
Apr 8, 2022
Mailmap update I noticed there are a lot of contributors who appear multiple times in https://thanks.rust-lang.org/rust/all-time/, which makes their "rank" on that page inaccurate. For example Nick Cameron currently appears at rank 21 with 2010 contributions and at rank 27 with 1287 contributions, because some of those are from nrc&rust-lang#8288;``@ncameron.org`` and some from ncameron&rust-lang#8288;``@mozilla.com.`` In reality Nick's rank would be 11 if counted correctly, which is a large difference. Solving this in a totally automated way is tricky because it involves figuring out whether Nick is 1 person with multiple emails, or is 2 people sharing the same name. This PR addresses a subset of the cases: only where a person has committed under multiple names using the same email. This is still not something that can be totally automated (e.g. by modifying https://github.com/rust-lang/thanks to dedup by email instead of name+email) because: - Some emails are not necessarily unique to one contributor, such as `ubuntu@localhost`. - It involves some judgement and mindfulness in picking the "canonical name" among the names used with a particular email. This is the name that will appear on thanks.rust-lang.org. Humans change their names sometimes and can be sensitive or picky about the use of names that are no longer preferred. For the purpose of this PR, I've tried to stick to the following heuristics which should be unobjectionable: - If one of the names is currently set as the display name on the contributor's GitHub profile, prefer that name. - If one of the names is used exclusively over the others in chronologically newer pull requests, prefer the newest name. - If one of the names has whitespace and the other doesn't (i.e. is username-like), such as `Foo Bar` vs `FooBar` or `foobar` or `foo-bar123`, but otherwise closely resemble one another, then prefer the human-like name. - If none of the above suffice in determining a canonical name and the contributor has some other name set on their GitHub profile, use the name from the GitHub profile. - If no name on their GitHub profile but the profile links to their personal website which unambiguously identifies their preferred name, then use that name. I'm also thinking about how to handle cases like Nick's, but that will be a project for a different PR. Basically I'd like to be able to find cases of the same person making commits that differ in name *and* email by looking at all the commits present in pull requests opened by the same GitHub user. <details> <summary>script</summary> ```toml [dependencies] anyhow = "1.0" git2 = "0.14" mailmap = "0.1" ``` ```rust use anyhow::{bail, Context, Result}; use git2::{Commit, Oid, Repository}; use mailmap::{Author, Mailmap}; use std::collections::{BTreeMap as Map, BTreeSet as Set}; use std::fmt::{self, Debug}; use std::fs; use std::path::Path; const REPO: &str = "/git/rust"; fn main() -> Result<()> { let repo = Repository::open(REPO)?; let head_oid = repo .head()? .target() .context("expected head to be a direct reference")?; let head = repo.find_commit(head_oid)?; let mailmap_path = Path::new(REPO).join(".mailmap"); let mailmap_contents = fs::read_to_string(mailmap_path)?; let mailmap = match Mailmap::from_string(mailmap_contents) { Ok(mailmap) => mailmap, Err(box_error) => bail!("{}", box_error), }; let mut history = Set::new(); let mut merges = Vec::new(); let mut authors = Set::new(); let mut emails = Map::new(); let mut all_authors = Set::new(); traverse_left(head, &mut history, &mut merges, &mut authors, &mailmap)?; while let Some((commit, i)) = merges.pop() { let right = commit.parents().nth(i).unwrap(); authors.clear(); traverse_left(right, &mut history, &mut merges, &mut authors, &mailmap)?; for author in &authors { all_authors.insert(author.clone()); if !author.email.is_empty() { emails .entry(author.email.clone()) .or_insert_with(Map::new) .entry(author.name.clone()) .or_insert_with(Set::new); } } if let Some(summary) = commit.summary() { if let Some(pr) = parse_summary(summary)? { for author in &authors { if !author.email.is_empty() { emails .get_mut(&author.email) .unwrap() .get_mut(&author.name) .unwrap() .insert(pr); } } } } } for (email, names) in emails { if names.len() > 1 { println!("<{}>", email); for (name, prs) in names { let prs = DebugSet(prs.iter().rev()); println!(" {} {:?}", name, prs); } } } eprintln!("{} commits", history.len()); eprintln!("{} authors", all_authors.len()); Ok(()) } fn traverse_left<'repo>( mut commit: Commit<'repo>, history: &mut Set<Oid>, merges: &mut Vec<(Commit<'repo>, usize)>, authors: &mut Set<Author>, mailmap: &Mailmap, ) -> Result<()> { loop { let oid = commit.id(); if !history.insert(oid) { return Ok(()); } let author = author(mailmap, &commit); let is_bors = author.name == "bors" && author.email == "[email protected]"; if !is_bors { authors.insert(author); } let mut parents = commit.parents(); let parent = match parents.next() { Some(parent) => parent, None => return Ok(()), }; for i in 1..1 + parents.len() { merges.push((commit.clone(), i)); } commit = parent; } } fn parse_summary(summary: &str) -> Result<Option<PullRequest>> { let mut rest = None; for prefix in [ "Auto merge of #", "Merge pull request #", " Manual merge of #", "auto merge of #", "auto merge of pull req #", "rollup merge of #", "Rollup merge of #", "Rollup merge of #", "Rollup merge of ", "Merge PR #", "Merge #", "Merged #", ] { if summary.starts_with(prefix) { rest = Some(&summary[prefix.len()..]); break; } } let rest = match rest { Some(rest) => rest, None => return Ok(None), }; let end = rest.find([' ', ':']).unwrap_or(rest.len()); let number = match rest[..end].parse::<u32>() { Ok(number) => number, Err(err) => { eprintln!("{}", summary); bail!(err); } }; Ok(Some(PullRequest(number))) } fn author(mailmap: &Mailmap, commit: &Commit) -> Author { let signature = commit.author(); let name = String::from_utf8_lossy(signature.name_bytes()).into_owned(); let email = String::from_utf8_lossy(signature.email_bytes()).into_owned(); mailmap.canonicalize(&Author { name, email }) } #[derive(Copy, Clone, Ord, PartialOrd, Eq, PartialEq)] struct PullRequest(u32); impl Debug for PullRequest { fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result { write!(formatter, "#{}", self.0) } } struct DebugSet<T>(T); impl<T> Debug for DebugSet<T> where T: Iterator + Clone, T::Item: Debug, { fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result { formatter.debug_set().entries(self.0.clone()).finish() } } ``` </details>
Dylan-DPC
added a commit
to Dylan-DPC/rust
that referenced
this pull request
Apr 8, 2022
Mailmap update I noticed there are a lot of contributors who appear multiple times in https://thanks.rust-lang.org/rust/all-time/, which makes their "rank" on that page inaccurate. For example Nick Cameron currently appears at rank 21 with 2010 contributions and at rank 27 with 1287 contributions, because some of those are from nrc&rust-lang#8288;```@ncameron.org``` and some from ncameron&rust-lang#8288;```@mozilla.com.``` In reality Nick's rank would be 11 if counted correctly, which is a large difference. Solving this in a totally automated way is tricky because it involves figuring out whether Nick is 1 person with multiple emails, or is 2 people sharing the same name. This PR addresses a subset of the cases: only where a person has committed under multiple names using the same email. This is still not something that can be totally automated (e.g. by modifying https://github.com/rust-lang/thanks to dedup by email instead of name+email) because: - Some emails are not necessarily unique to one contributor, such as `ubuntu@localhost`. - It involves some judgement and mindfulness in picking the "canonical name" among the names used with a particular email. This is the name that will appear on thanks.rust-lang.org. Humans change their names sometimes and can be sensitive or picky about the use of names that are no longer preferred. For the purpose of this PR, I've tried to stick to the following heuristics which should be unobjectionable: - If one of the names is currently set as the display name on the contributor's GitHub profile, prefer that name. - If one of the names is used exclusively over the others in chronologically newer pull requests, prefer the newest name. - If one of the names has whitespace and the other doesn't (i.e. is username-like), such as `Foo Bar` vs `FooBar` or `foobar` or `foo-bar123`, but otherwise closely resemble one another, then prefer the human-like name. - If none of the above suffice in determining a canonical name and the contributor has some other name set on their GitHub profile, use the name from the GitHub profile. - If no name on their GitHub profile but the profile links to their personal website which unambiguously identifies their preferred name, then use that name. I'm also thinking about how to handle cases like Nick's, but that will be a project for a different PR. Basically I'd like to be able to find cases of the same person making commits that differ in name *and* email by looking at all the commits present in pull requests opened by the same GitHub user. <details> <summary>script</summary> ```toml [dependencies] anyhow = "1.0" git2 = "0.14" mailmap = "0.1" ``` ```rust use anyhow::{bail, Context, Result}; use git2::{Commit, Oid, Repository}; use mailmap::{Author, Mailmap}; use std::collections::{BTreeMap as Map, BTreeSet as Set}; use std::fmt::{self, Debug}; use std::fs; use std::path::Path; const REPO: &str = "/git/rust"; fn main() -> Result<()> { let repo = Repository::open(REPO)?; let head_oid = repo .head()? .target() .context("expected head to be a direct reference")?; let head = repo.find_commit(head_oid)?; let mailmap_path = Path::new(REPO).join(".mailmap"); let mailmap_contents = fs::read_to_string(mailmap_path)?; let mailmap = match Mailmap::from_string(mailmap_contents) { Ok(mailmap) => mailmap, Err(box_error) => bail!("{}", box_error), }; let mut history = Set::new(); let mut merges = Vec::new(); let mut authors = Set::new(); let mut emails = Map::new(); let mut all_authors = Set::new(); traverse_left(head, &mut history, &mut merges, &mut authors, &mailmap)?; while let Some((commit, i)) = merges.pop() { let right = commit.parents().nth(i).unwrap(); authors.clear(); traverse_left(right, &mut history, &mut merges, &mut authors, &mailmap)?; for author in &authors { all_authors.insert(author.clone()); if !author.email.is_empty() { emails .entry(author.email.clone()) .or_insert_with(Map::new) .entry(author.name.clone()) .or_insert_with(Set::new); } } if let Some(summary) = commit.summary() { if let Some(pr) = parse_summary(summary)? { for author in &authors { if !author.email.is_empty() { emails .get_mut(&author.email) .unwrap() .get_mut(&author.name) .unwrap() .insert(pr); } } } } } for (email, names) in emails { if names.len() > 1 { println!("<{}>", email); for (name, prs) in names { let prs = DebugSet(prs.iter().rev()); println!(" {} {:?}", name, prs); } } } eprintln!("{} commits", history.len()); eprintln!("{} authors", all_authors.len()); Ok(()) } fn traverse_left<'repo>( mut commit: Commit<'repo>, history: &mut Set<Oid>, merges: &mut Vec<(Commit<'repo>, usize)>, authors: &mut Set<Author>, mailmap: &Mailmap, ) -> Result<()> { loop { let oid = commit.id(); if !history.insert(oid) { return Ok(()); } let author = author(mailmap, &commit); let is_bors = author.name == "bors" && author.email == "[email protected]"; if !is_bors { authors.insert(author); } let mut parents = commit.parents(); let parent = match parents.next() { Some(parent) => parent, None => return Ok(()), }; for i in 1..1 + parents.len() { merges.push((commit.clone(), i)); } commit = parent; } } fn parse_summary(summary: &str) -> Result<Option<PullRequest>> { let mut rest = None; for prefix in [ "Auto merge of #", "Merge pull request #", " Manual merge of #", "auto merge of #", "auto merge of pull req #", "rollup merge of #", "Rollup merge of #", "Rollup merge of #", "Rollup merge of ", "Merge PR #", "Merge #", "Merged #", ] { if summary.starts_with(prefix) { rest = Some(&summary[prefix.len()..]); break; } } let rest = match rest { Some(rest) => rest, None => return Ok(None), }; let end = rest.find([' ', ':']).unwrap_or(rest.len()); let number = match rest[..end].parse::<u32>() { Ok(number) => number, Err(err) => { eprintln!("{}", summary); bail!(err); } }; Ok(Some(PullRequest(number))) } fn author(mailmap: &Mailmap, commit: &Commit) -> Author { let signature = commit.author(); let name = String::from_utf8_lossy(signature.name_bytes()).into_owned(); let email = String::from_utf8_lossy(signature.email_bytes()).into_owned(); mailmap.canonicalize(&Author { name, email }) } #[derive(Copy, Clone, Ord, PartialOrd, Eq, PartialEq)] struct PullRequest(u32); impl Debug for PullRequest { fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result { write!(formatter, "#{}", self.0) } } struct DebugSet<T>(T); impl<T> Debug for DebugSet<T> where T: Iterator + Clone, T::Item: Debug, { fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result { formatter.debug_set().entries(self.0.clone()).finish() } } ``` </details>
flip1995
pushed a commit
to flip1995/rust
that referenced
this pull request
Jul 18, 2022
fixes: rust-lang#8288 --- changelog: Allowing [`qustion_mark`] lint to check `if let` expressions that immediatly return unwrapped value
flip1995
pushed a commit
to flip1995/rust
that referenced
this pull request
Jul 18, 2022
Simplify if let statements fixes: rust-lang#8288 --- changelog: Allowing [`qustion_mark`] lint to check `if let` expressions that immediatly return unwrapped value
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an alternative version to #8268, where instead of transitioning to
get()completely, I transitioned tounwrap()completely.My reasoning for also opening this PR is that having two different functions with identical behavior on a common datatype is bad for consistency and confusing for users, and should be solved as soon as possible. The fact that apparently half the code uses
get(), and the other halfunwrap()only makes it worse.If the final naming decision ends up different, there needs to be a big renaming anyway, but until then it should at least be consistent.
See Overloaded
Addon Option has arguably the wrong behavior #6002, especially my last comment.unwrapinsteadSee also RFC: Boxes vs Containers - Naming schemes for element access #7887.
Todo:
Adding testcases for all function in the three modules. Even without the few functions I added, the coverage wasn't complete to begin with. But I'd rather do that as a follow up PR, I've touched to much code here already, need to go through them again later.