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

BoolError: allow owning the message #419

Merged
merged 1 commit into from Jan 4, 2019

Conversation

Projects
None yet
4 participants
@fengalin
Copy link
Contributor

fengalin commented Dec 31, 2018

BoolError can currently only be built from a &'static str.
This prevents the user from dynamically building the message.

Thanks to @sdroege for suggesting to use Cow.

It's also an opportunity to keep track of the location where the error was built.

This causes breaking changes in some callers but I think previous implementation was not future-proof, so the change should be done IMHO.

Show resolved Hide resolved src/error.rs Outdated
Show resolved Hide resolved src/error.rs Outdated

@fengalin fengalin force-pushed the fengalin:bool_error_cow branch from e5436b9 to b98394d Jan 2, 2019

@@ -133,25 +134,66 @@ pub trait ErrorDomain: Copy {

/// Generic error used for functions that fail without any further information
#[derive(Debug)]
pub struct BoolError(pub &'static str);
pub struct BoolError(pub Cow<'static, str>);

This comment has been minimized.

@sdroege

sdroege Jan 2, 2019

Member

Maybe while we extend this, we could also add a field for file!(), module_path!() and line!() and make a constructor-macro that fills that in?

This comment has been minimized.

@sdroege

This comment has been minimized.

@fengalin

fengalin Jan 2, 2019

Author Contributor

Good idea!

This comment has been minimized.

@fengalin

fengalin Jan 2, 2019

Author Contributor

See the new commit. I'm not convinced by the macro name: glib_result_from_glib. Any idea? I though glib_bool_error_result_from_glib was a bit too long. Maybe glib_result_from_glib_bool?

This comment has been minimized.

@sdroege

sdroege Jan 3, 2019

Member

glib_bool_error!(message) and glib_bool_error_from_gboolean!(message, b) maybe?

This comment has been minimized.

@fengalin

fengalin Jan 3, 2019

Author Contributor

I would tend to use glib_result_from_gboolean instead of glib_bool_error_from_gboolean since the return value is a Result.

Why do you prefer putting the boolean as the last argument? (I kept the order from the signature of BoolError::from_glib).

This comment has been minimized.

@sdroege

sdroege Jan 3, 2019

Member

I don't mind either way :)

fengalin added a commit to fengalin/gtk that referenced this pull request Jan 2, 2019

Show resolved Hide resolved src/error.rs Outdated

@fengalin fengalin force-pushed the fengalin:bool_error_cow branch from 036dbaf to 58055f5 Jan 3, 2019

@fengalin

This comment has been minimized.

Copy link
Contributor Author

fengalin commented Jan 3, 2019

@GuillaumeGomez & @EPashkin what do you think about these changes? If it's OK with you, I'll modify gtk-rs/gtk.

Note: CI will keep failing until gtk-rs/gtk is updated.

let from_dynamic_msg = glib_bool_error!("{} message", "Dynamic");
assert_eq!(from_dynamic_msg.description(), "Dynamic message");

let static_res = glib_result_from_gboolean!(glib_ffi::GFALSE, "Static message");

This comment has been minimized.

@EPashkin

EPashkin Jan 3, 2019

Member

Please add test for glib_ffi::GTRUE just for completeness

This comment has been minimized.

@fengalin

fengalin Jan 3, 2019

Author Contributor

Yes, you're right!

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jan 3, 2019

@fengalin Thanks, other that small nit all looks good for me.

@fengalin fengalin force-pushed the fengalin:bool_error_cow branch from 58055f5 to a406aac Jan 3, 2019

fengalin added a commit to fengalin/gtk that referenced this pull request Jan 3, 2019

BoolError: allow owning the message
`BoolError` can currently only be built from a `&'static str`.
This prevents the user from dynamically building the message.

This is also an opportunity to trace the location where the
error was built.

@fengalin fengalin force-pushed the fengalin:bool_error_cow branch from a406aac to 8a6b575 Jan 3, 2019

fengalin added a commit to fengalin/gtk that referenced this pull request Jan 3, 2019

@fengalin

This comment has been minimized.

Copy link
Contributor Author

fengalin commented Jan 3, 2019

I missed something: glib::error::BoolError::from_glib is used in many auto-generated files in gstreamer-rs.

Please wait before merging, I need to figure out how to migrate to the new macro.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jan 3, 2019

@fengalin

This comment has been minimized.

Copy link
Contributor Author

fengalin commented Jan 3, 2019

@fengalin

This comment has been minimized.

Copy link
Contributor Author

fengalin commented Jan 3, 2019

@sdroege: gstreamer-rs is currently @gtk-rs/gir@811e711. I see there are commits such as Generate code using GString instead of String after this. I presume, this would lead to issues if I update Gir. What do you think?

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jan 3, 2019

@sdroege: gstreamer-rs is currently @gtk-rs/gir@811e711. I see there are commits such as Generate code using GString instead of String after this. I presume, this would lead to issues if I update Gir. What do you think?

That's fine, regenerating with newer gir shouldn't cause any problems.

@fengalin

This comment has been minimized.

Copy link
Contributor Author

fengalin commented Jan 4, 2019

That's fine, regenerating with newer gir shouldn't cause any problems.

Yes indeed, it's fine. I checked gstreamer-rs/gstreamer with my modifications and unit tests are all good.

@EPashkin, it should be ready to merge now. See also gtk-rs/gir#687 & gtk-rs/gtk#751.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jan 4, 2019

@fengalin Yes, i checked all 3 PRs and it works fine, thanks.
@GuillaumeGomez 👍 for this.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Jan 4, 2019

Compilation errors seem to be expected. Code looks good.

@fengalin: Thanks!

@GuillaumeGomez GuillaumeGomez merged commit c2780be into gtk-rs:master Jan 4, 2019

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details

fengalin added a commit to fengalin/gtk that referenced this pull request Jan 4, 2019

@fengalin fengalin deleted the fengalin:bool_error_cow branch Jan 4, 2019

gstreamer-github pushed a commit to sdroege/gstreamer-rs that referenced this pull request Jan 4, 2019

vhdirk pushed a commit to vhdirk/gtk-rs that referenced this pull request Jan 16, 2019

GuillaumeGomez added a commit to GuillaumeGomez/gtk that referenced this pull request Feb 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.