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

Make ObjectType::new() optional too #416

Merged
merged 1 commit into from Dec 19, 2018

Conversation

Projects
None yet
3 participants
@sdroege
Copy link
Member

sdroege commented Dec 18, 2018

We either require new() or new_with_type() to be implemented. Making it
mandatory to implement new() in any case causes ugly code in subclasses
that only want to implement new_with_type().


This has the negative side-effect that there's no compiler warning if neither is implemented... but users will notice very fast because their code just does not work at all due to the unimplemented!() default implementation.

Compared to ugly code like this I really prefer that.

Make ObjectType::new() optional too
We either require new() or new_with_type() to be implemented. Making it
mandatory to implement new() in any case causes ugly code in subclasses
that only want to implement new_with_type().
@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Dec 19, 2018

I ok with this solution,
but why not change return type to Option<Self> and produce meaningful message in instance_init?

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Dec 19, 2018

Because it's never correct to return None here and we can only possibly panic then.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Dec 19, 2018

Ok

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Dec 19, 2018

Thanks!

@GuillaumeGomez GuillaumeGomez merged commit d4d5c91 into gtk-rs:master Dec 19, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.