-
Notifications
You must be signed in to change notification settings - Fork 13.3k
unwrap_or
lint corrected
#78825
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
unwrap_or
lint corrected
#78825
Conversation
Some changes occurred in intra-doc-links. cc @jyn514 |
r? @lcnr (rust_highfive has picked a reviewer for you, use r? to override) |
@bors try @rust-timer queue It seems plausible that gating the method call could actually hurt in some cases (since it changes operation order somewhat). Probably doesn't matter in practice, though. |
Awaiting bors try build completion |
⌛ Trying commit 1c7498c3e57e25363d0b7d3a716f7e1234604c4f with merge 3de0a1b5392ebbbfc1249710da2f62232983e9a9... |
☀️ Try build successful - checks-actions |
Queued 3de0a1b5392ebbbfc1249710da2f62232983e9a9 with parent a601302, future comparison URL. |
Finished benchmarking try commit (3de0a1b5392ebbbfc1249710da2f62232983e9a9): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following changes are reversions of unwrap_or_else
to unwrap_or
.
Git does support bundling changes for multiple files into a single commit. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
fdf536f
to
7b22a30
Compare
@the8472 I rolled up all 20 of my changes into one revert commit. Yes, I have wasted my time with 20 one line commits and I really should learn from that. |
LGTM. I do however think 14 commits is a bit too much for this PR, r=me after you did some squashing here |
2cae639
to
d29869c
Compare
@lcnr Squashed this PR down to 4 commits and rebased it onto the latest master. |
Last 3 commits look like they'd just be noise in the commit log? Is there a reason why they were kept? |
The 2nd commit is a revert explaining why. I could merge the last 3 into one commit. |
d29869c
to
f5080ed
Compare
I do not think there should be more than 1 commit here, personally. I think it is useful to have standalone commits if they have distinct contents from other commits in the PR, which is not the case here. Generally in-PR reverts just don't make sense for preserving indefinitely. |
Ok, squashing into 1 commit |
f5080ed
to
10ec904
Compare
The discussion seems to have resolved that this lint is a bit "noisy" in that applying it in all places would result in a reduction in readability. A few of the trivial functions (like `Path::new`) are fine to leave outside of closures. The general rule seems to be that anything that is obviously an allocation (`Box`, `Vec`, `vec![]`) should be in a closure, even if it is a 0-sized allocation.
10ec904
to
261ca04
Compare
Thanks 👍 @bors r+ |
📌 Commit 261ca04 has been approved by |
@Nicholas-Baron btw, there were recently a bunch of clippy bugs fixed, so I would expect |
☀️ Test successful - checks-actions |
#78814 (comment)
This pull request fixes the lint from clippy where
unwrap_or
could be better done as aunwrap_or_else
or aunwrap_or_default
.