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

Migrate property getters to new signature for `Value::get` #825

Merged
merged 2 commits into from Aug 9, 2019

Conversation

@fengalin
Copy link
Contributor

fengalin commented Aug 8, 2019

... in consequence of gtk-rs/glib#513

src/codegen/property_body.rs Outdated Show resolved Hide resolved
@fengalin fengalin force-pushed the fengalin:value_get_result branch from d1e0dac to 7435824 Aug 8, 2019
@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Aug 8, 2019

Is this all ready from your side then? :) I'd give it another review later

@fengalin

This comment has been minimized.

Copy link
Contributor Author

fengalin commented Aug 8, 2019

Is this all ready from your side then?

Not yet, I'm checking the other crates now :)

@fengalin fengalin force-pushed the fengalin:value_get_result branch from f66f79f to fbc818c Aug 8, 2019
@fengalin

This comment has been minimized.

Copy link
Contributor Author

fengalin commented Aug 8, 2019

It should be good to go now. So far, I checked:

  • gtk-rs/gtk: generation & unit tests with features subclassing,futures (which also pulls atk, gdk, gdk-pixbuf & gio)
  • gstreamer-rs/gstreamer: generation & unit tests with features subclassing,futures,serde.
@fengalin

This comment has been minimized.

Copy link
Contributor Author

fengalin commented Aug 9, 2019

Guys, when you get time, could you review & merge this one please? This will allow me to regen the other crates with the actual gir version.

@@ -138,7 +138,10 @@ impl Builder {
} else {
".unwrap()"
};
body.push(Chunk::Custom(format!("value.get(){}", unwrap)));
body.push(Chunk::Custom(format!(
"value.get().expect(\"Return Value for property `{}` getter\"){}",

This comment has been minimized.

Copy link
@EPashkin

EPashkin Aug 9, 2019

Member

This error not tell about actual reason.
IMHO just unwrap give better error text

This comment has been minimized.

Copy link
@fengalin

fengalin Aug 9, 2019

Author Contributor

Yes it does... somewhat. See this discussion: #825 (review) (you will need to click "Show resolved").

This comment has been minimized.

Copy link
@EPashkin

EPashkin Aug 9, 2019

Member

Sorry, I missed that discussion.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Aug 9, 2019

@fengalin Thanks, I will merge then

@EPashkin EPashkin merged commit 11e59a0 into gtk-rs:master Aug 9, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@fengalin

This comment has been minimized.

Copy link
Contributor Author

fengalin commented Aug 9, 2019

Thanks @EPashkin

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