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

Removed wrong usage of default from the Option API #10854

Closed

Conversation

Kimundi
Copy link
Member

@Kimundi Kimundi commented Dec 8, 2013

This implements parts of the changes to Option I proposed and discussed in this thread: https://mail.mozilla.org/pipermail/rust-dev/2013-November/006254.html, and on IRC.

In short, the string "default" should not be used in any context that has nothing to do with the std::default::Default trait.

This PR consists of this change:

  • Renamed map_default -> map_or and mutate_default -> mutate_or_set.

@huonw
Copy link
Member

huonw commented Dec 8, 2013

I feel like the arg-swap is a negative. The closure can be a large block of code, which leaves the default value hidden behind it. shrugs

@bill-myers
Copy link
Contributor

Shouldn't there be a map_or_else that takes a closure for the default value?

Typical use is when the default value needs memory allocations to be constructed, or needs to be retrieved with a system call or a communication mechanism.

@alexcrichton
Copy link
Member

This is a change to some core types which needs to be discussed.

@Kimundi
Copy link
Member Author

Kimundi commented Dec 8, 2013

@bill-myers It's hard to draw the line there - if we just added everything that might be useful we might end up with a lot of maintenance burden for functions no one ever uses - in this case map_or_else does not exists, and is thus not used by any part of the codebase, while map_or is used.

In any case, just like map_or(f, a) is shortform for map(f).unwrap_or(a), map_or_else(f, g) would be shortform for map(f).or_else(|| Some(g())).unwrap(), which, while ugly, is at least expressible with the existing methods.


EDIT: With this I don't mean to discourage adding such a function though, there are a few API holes like this, for example an expect that accepts a closure for computing the failure message.

@alexcrichton
Copy link
Member

This is on the next meeting agenda

@alexcrichton
Copy link
Member

We decided in today's meeting that the renaming is a good idea, but the reordering of parameters is not as favorable. Would you mind removing the commit with the reordering of the parameters?

@alexcrichton
Copy link
Member

Ah, I should link to the notes as well: https://github.com/mozilla/rust/wiki/Meeting-weekly-2014-01-07

bors added a commit that referenced this pull request Jan 8, 2014
…lexcrichton

This implements parts of the changes to `Option` I proposed and discussed in this thread: https://mail.mozilla.org/pipermail/rust-dev/2013-November/006254.html, and on IRC.

In short, the string "default" should not be used in any context that has nothing to do with the `std::default::Default` trait.

This PR consists of this change:
- Renamed `map_default -> map_or` and `mutate_default -> mutate_or_set`.
@bors bors closed this Jan 8, 2014
rlane added a commit to rlane/glfw-rs that referenced this pull request Jan 8, 2014
rlane added a commit to rlane/glfw-rs that referenced this pull request Jan 8, 2014
rlane added a commit to rlane/glfw-rs that referenced this pull request Jan 28, 2014
flip1995 pushed a commit to flip1995/rust that referenced this pull request Feb 8, 2024
flip1995 pushed a commit to flip1995/rust that referenced this pull request Feb 8, 2024
…t-closure-for-method-calls, r=blyxyas

[fix] [`redundant_closure_for_method_calls`] Suggest relative paths for local modules

Fixes rust-lang#10854.

Currently, `redundant_closure_for_method_calls` suggest incorrect paths when a method defined on a struct within inline mod is referenced (see the description in the aforementioned issue for an example; also see [this playground link](https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=f7d3c5b2663c9bd3ab7abdb0bd38ee43) for the current-version output for the test cases added in this PR). It will now try to construct a relative path path to the module and suggest it instead.

changelog: [`redundant_closure_for_method_calls`] Fix incorrect path suggestions for types within local modules
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants