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

Be more precise when setting a property with invalid type #214

Merged
merged 1 commit into from Aug 18, 2017

Conversation

Projects
None yet
4 participants
@fengalin
Contributor

fengalin commented Aug 17, 2017

When setting an Object property with a valid name but an invalid type, "Invalid property name" is returned. This modification allows returning "Invalid property type" in this case.

Changes return type for Object::has_property.

Show outdated Hide outdated src/object.rs Outdated
@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Aug 17, 2017

Member

This is awesome! Just a nit and it's good for me.

cc @EPashkin @sdroege

Member

GuillaumeGomez commented Aug 17, 2017

This is awesome! Just a nit and it's good for me.

cc @EPashkin @sdroege

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Aug 17, 2017

Member

It's fine to change API IMHO, these functions are all new and in no release yet

Member

sdroege commented Aug 17, 2017

It's fine to change API IMHO, these functions are all new and in no release yet

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Aug 17, 2017

Member

I.e., please just change the existing function :)

Member

sdroege commented Aug 17, 2017

I.e., please just change the existing function :)

@fengalin

This comment has been minimized.

Show comment
Hide comment
@fengalin

fengalin Aug 17, 2017

Contributor

All right, I'll do that!

Contributor

fengalin commented Aug 17, 2017

All right, I'll do that!

@fengalin

This comment has been minimized.

Show comment
Hide comment
@fengalin

fengalin Aug 17, 2017

Contributor

By the way, any suggestion on the "ugly part"?

Contributor

fengalin commented Aug 17, 2017

By the way, any suggestion on the "ugly part"?

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Aug 17, 2017

Member

What ugly part?

Member

sdroege commented Aug 17, 2017

What ugly part?

@fengalin

This comment has been minimized.

Show comment
Hide comment
@fengalin

fengalin Aug 17, 2017

Contributor

It used to be:

        match ptype {
            None => Err(BoolError("Invalid property name")),
            Some(ptype) => if ptype == type_ {
                    Ok(())
                } else {
                    Err(BoolError("Invalid property type"))
                },
        }

I must admit, I was not too proud of it. Then upon @GuillaumeGomez request, I proposed this:

        if let Some(ptype) = ptype {
            if ptype == type_ {
                Ok(())
            } else {
                Err(BoolError("Invalid property type"))
            }
        } else {
            Err(BoolError("Invalid property name"))
        }

But I'm still not convinced.

Contributor

fengalin commented Aug 17, 2017

It used to be:

        match ptype {
            None => Err(BoolError("Invalid property name")),
            Some(ptype) => if ptype == type_ {
                    Ok(())
                } else {
                    Err(BoolError("Invalid property type"))
                },
        }

I must admit, I was not too proud of it. Then upon @GuillaumeGomez request, I proposed this:

        if let Some(ptype) = ptype {
            if ptype == type_ {
                Ok(())
            } else {
                Err(BoolError("Invalid property type"))
            }
        } else {
            Err(BoolError("Invalid property name"))
        }

But I'm still not convinced.

@fengalin

This comment has been minimized.

Show comment
Hide comment
@fengalin

fengalin Aug 17, 2017

Contributor

Never mind... I got it now.

Contributor

fengalin commented Aug 17, 2017

Never mind... I got it now.

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Aug 17, 2017

Member

Looks good to me

Member

sdroege commented Aug 17, 2017

Looks good to me

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Aug 18, 2017

Member

LGFM, only commit title better changed.

Member

EPashkin commented Aug 18, 2017

LGFM, only commit title better changed.

@fengalin

This comment has been minimized.

Show comment
Hide comment
@fengalin

fengalin Aug 18, 2017

Contributor

Do you want me to squash the commits and keep the first commit message only?

Contributor

fengalin commented Aug 18, 2017

Do you want me to squash the commits and keep the first commit message only?

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Aug 18, 2017

Member

Yes please!

Member

GuillaumeGomez commented Aug 18, 2017

Yes please!

Be more precise when setting a property with invalid type
When setting an `Object` property with a valid name but an invalid type, "Invalid property name" is returned. This modification allows returning "Invalid property type" in this case.
Changes return type for `Object::has_property`.
@fengalin

This comment has been minimized.

Show comment
Hide comment
@fengalin

fengalin Aug 18, 2017

Contributor

Not sure if you want me to do something else on this one.

@EPashkin: shall I enhance "Be more precise when setting a property with invalid type"?

Contributor

fengalin commented Aug 18, 2017

Not sure if you want me to do something else on this one.

@EPashkin: shall I enhance "Be more precise when setting a property with invalid type"?

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Aug 18, 2017

Member

Yes, after squash all good for me. Thanks.

Member

EPashkin commented Aug 18, 2017

Yes, after squash all good for me. Thanks.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Aug 18, 2017

Member

Well, he already squashed so let's merge. Thanks!

Member

GuillaumeGomez commented Aug 18, 2017

Well, he already squashed so let's merge. Thanks!

@GuillaumeGomez GuillaumeGomez merged commit f2c9407 into gtk-rs:master Aug 18, 2017

2 checks passed

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

@fengalin fengalin deleted the fengalin:property_type_error branch Aug 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment