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

Relax Fn trait bounds in Command & Action #1409

Merged
merged 2 commits into from
Aug 17, 2022
Merged

Conversation

wuxianucw
Copy link
Contributor

In my practical use, I found that many methods in Command and Action requires a strict Fn trait bound (and even more like 'static and Clone), which troubled me a lot. I think in some case such strict restrictions is no need.

In this PR, I tried my best to relax these restrictions. At least for me, it was very helpful. Hope it can help more friends working around async.

BTW, I notice that the different types of Action and whether Command contains only one Action can affect the minimal restriction. Maybe we can introduce more methods such as map_single, map_future, etc. to make it more easy while mapping Command and Action.

@hecrj
Copy link
Member

hecrj commented Aug 11, 2022

which troubled me a lot

Could you elaborate a bit? Maybe provide some examples?

@wuxianucw
Copy link
Contributor Author

which troubled me a lot

Could you elaborate a bit? Maybe provide some examples?

The main issue is that I can only use Fn for Command::perform. Simply I write the code like this:

pub async fn my_async_operation() -> Result<String> { /* -- snip -- */ }
// -- snip --
let config: Config = /* a temporary value in this scope */;
Command::perform(my_async_operation(), move |r| Message::OperationFinished(config, r))

However, Rust will complain my closure doesn's impl Fn required by Command::perform. Since FutureExt::map just requires FnOnce, Command::perform shouldn't ask for a even more strict restriction like Fn if it do not really need, I think. Currently, other types of Action seems also not to need Fn for map.

@hecrj hecrj added this to the 0.5.0 milestone Aug 17, 2022
@hecrj hecrj added the feature New feature or request label Aug 17, 2022
... and revert `FnMut` usage.
@hecrj
Copy link
Member

hecrj commented Aug 17, 2022

FnOnce for Command::perform makes sense.

I don't see a good use case for FnMut in Command::map, so I have reverted those changes in 23229e0.

@hecrj hecrj added the shell label Aug 17, 2022
@hecrj hecrj merged commit f728d6c into iced-rs:master Aug 17, 2022
@wuxianucw
Copy link
Contributor Author

FnOnce for Command::perform makes sense.

I don't see a good use case for FnMut in Command::map, so I have reverted those changes in 23229e0.

Thanks! That do help a lot. I changed Fn to FnMut in Command::map just because FnMut is the most relaxed restriction at current time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request shell
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants