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

Bring back &str assignment to optional properties #1895

Merged
merged 1 commit into from
Jun 7, 2021

Conversation

ctron
Copy link
Contributor

@ctron ctron commented Jun 2, 2021

This drops the "optional" conversion concept in favor of directly converting to Option<T>.

Description

This drops the IntoOptPropValue trait, and uses a conversion to Option<T> instead. This brings back the ability to assign &str to an Option<String> or Option<Cow<'static, str>> property.

Fixes #1888

Checklist

  • I have run cargo make pr-flow
  • I have reviewed my own code
  • I have added tests

siku2
siku2 previously approved these changes Jun 4, 2021
Copy link
Member

@siku2 siku2 left a comment

Choose a reason for hiding this comment

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

I'm pretty sure there was a reason for the IntoOptPropValue trait. Part of it was that it makes it possible to distinguish between "optional" attributes and attributes which just happen to be Option's.
Most of the useful properties of IntoOptPropValue didn't work out though because of the lack of specialization... Given that, I believe it totally makes sense to simplify the system back to just IntoPropValue<Option<T>>.

Comment on lines 129 to 138
html! {
<MockComponent
cow_string="foo"
cow_string_opt="foo"
string="foo"
string_opt="foo"
r#usize=12
usize_opt=12
/>
};
Copy link
Member

Choose a reason for hiding this comment

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

Could you perhaps test the IntoPropValue macro directly instead of through the macro?
i.e. just:

IntoPropValue::<String>("foo");
IntoPropValue::<Option<String>>("foo");

Also, since these are essentially compile tests, we should consider using trybuild for these too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this to:

fn test_str() {
    let _: String = "foo".into_prop_value();
    let _: Option<String> = "foo".into_prop_value();
    let _: Cow<'static, str> = "foo".into_prop_value();
    let _: Option<Cow<'static, str>> = "foo".into_prop_value();
}

@ranile
Copy link
Member

ranile commented Jun 5, 2021

This should go in master instead of v0.18. There are a handful of commits to be cherry picked into master before this can be merged though.

@ctron
Copy link
Contributor Author

ctron commented Jun 7, 2021

This should go in master instead of v0.18. There are a handful of commits to be cherry picked into master before this can be merged though.

For me it is hard to understand where what the current state of this repository is. So apologies if I targeted the wrong branch. I will try to rebase this on master.

@ctron ctron changed the base branch from v0.18 to master June 7, 2021 10:27
@mergify mergify bot dismissed siku2’s stale review June 7, 2021 10:27

Pull request has been modified.

@ctron ctron requested a review from siku2 June 7, 2021 10:28
@ctron
Copy link
Contributor Author

ctron commented Jun 7, 2021

I changed the base for the PR to target master instead.

@siku2
Copy link
Member

siku2 commented Jun 7, 2021

Hm, what's going on with the unit tests?

@ranile
Copy link
Member

ranile commented Jun 7, 2021

If I were to guess, the stderr files need to be updated.

@ctron ctron force-pushed the feature/fix_opt_props_1 branch 4 times, most recently from f30be11 to 4347b00 Compare June 7, 2021 13:32
This drops the "optional" conversion concept in favor of directly
converting to `Option<T>`.

fixes yewstack#1888
@ctron
Copy link
Contributor Author

ctron commented Jun 7, 2021

If I were to guess, the stderr files need to be updated.

Yes, I struggled a bit with these. But it looks green now.

@siku2 siku2 merged commit b03f02f into yewstack:master Jun 7, 2021
@ctron ctron deleted the feature/fix_opt_props_1 branch June 8, 2021 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to assign to Option<String> and Option<Cow<'static, str>>
3 participants