Skip to content
This repository was archived by the owner on Jun 8, 2021. It is now read-only.

Value::get: return a Result to account for type mismatch... #513

Merged
merged 7 commits into from
Aug 11, 2019
Merged

Value::get: return a Result to account for type mismatch... #513

merged 7 commits into from
Aug 11, 2019

Conversation

fengalin
Copy link
Contributor

@fengalin fengalin commented Aug 7, 2019

... and introduce get_some for types implementing FromValue.

First commit is for non-regression validation.

I'll update the other crates (gtk-rs & gstreamer-rs) when this is merged.

Fixes #510

... plus adjust unit and documentation tests.
@fengalin
Copy link
Contributor Author

fengalin commented Aug 7, 2019

Not ready yet! I still hope to find a good name for get_some methods :)

Align to usual conventions for `Error`s in the standard library.
@fengalin
Copy link
Contributor Author

fengalin commented Aug 8, 2019

I think we should keep get_some eventually. I can't find a better short replacement.

I am worried about the CI error on Windows 32bits: it seems like this platform doesn't like #[should_panic] attributes in tests. Any of you guys seen this before?

@sdroege
Copy link
Member

sdroege commented Aug 8, 2019

Probably related to using the mingw toolchain. Not sure what to do about that, the Windows CI is generally borderline broken (see also GTK failing).

@sdroege
Copy link
Member

sdroege commented Aug 8, 2019

Maybe blacklist that test on Windows for the time being.

@EPashkin
Copy link
Member

EPashkin commented Aug 8, 2019

There also error on installing mingw-w64-i686-glib2, but it was not passed build too:
https://ci.appveyor.com/project/GuillaumeGomez/glib/builds/26555598/job/f90x0sqsfokbj90i#L103

They break CI on this plateform:

```
fatal runtime error: failed to initiate panic, error 5
```
@fengalin
Copy link
Contributor Author

fengalin commented Aug 8, 2019

Maybe blacklist that test on Windows for the time being

I'm not sure if there is a better way to do this: I added a #[cfg(..)] clause before the 2 tests that #[should_panic].

There also error on installing mingw-w64-i686-glib2

Yup! It seems kind of broken ATM.

@fengalin
Copy link
Contributor Author

fengalin commented Aug 8, 2019

CI on Windows 32bits is now aligned with the other platforms.

I think the next step would be to merge gtk-rs/gir#825 so that I can regen the depending crates.

@sdroege
Copy link
Member

sdroege commented Aug 8, 2019

This PR here looks good to me in any case, thanks!

If you're interested you could also update all the BoolError in our manual code (like in the set_property() function) to proper error types :)

@fengalin
Copy link
Contributor Author

fengalin commented Aug 8, 2019

If you're interested you could also update all the BoolError in our manual code (like in the set_property() function) to proper error types :)

Ok, I'll add it to my todo list :) But I had other glib-related plans actually. I'll post a proposal when I'm done with the Value::get changes.

@sdroege
Copy link
Member

sdroege commented Aug 9, 2019

Ok, I'll add it to my todo list :) But I had other glib-related plans actually. I'll post a proposal when I'm done with the Value::get changes.

No problem :)

@EPashkin
Copy link
Member

EPashkin commented Aug 9, 2019

As result code looks like this:

let s = model
                    .get_value(&iter, 1)
                    .get::<String>()
                    .expect("Value is not string"),
                    .expect("Couldn't get string value"),

Maybe we try include None to error?

pub enum GetError {
    Nothing,
    WrongType{requested: Type, actual: Type},
}

@fengalin
Copy link
Contributor Author

fengalin commented Aug 9, 2019

Maybe we try include None to error?

The None case is not necessarily an error. There are cases for which you will want an optional String. See for instance this test case in subclass/object.rs.

For types that don't allow storing a None variant, we can use get_some.

@EPashkin
Copy link
Member

EPashkin commented Aug 9, 2019

Yes, with my proposal most of code will be use .ok() to get Option<T> so type error will be not handled correctly.
👍 for current version.
@fengalin Thanks

@fengalin
Copy link
Contributor Author

gtk-rs/examples#257 all green!

I think we can merge this one and I'll proceed with the direct dependents (gio, atk, gdk-pixbuf), then the others...

@sdroege
Copy link
Member

sdroege commented Aug 11, 2019

All good for me, but needs @GuillaumeGomez to merge all the trivial regen PRs :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants