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

Add glib::BoolError for use as return value of possibly failing functions returning booleans #169

Merged
merged 5 commits into from May 21, 2017

Conversation

Projects
None yet
3 participants
@sdroege
Member

sdroege commented May 17, 2017

gtk-rs/gir#368

And example usage in utils::setenv(). Code generator changes come tomorrow when this is all ok.

How do you handle API changes?

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 18, 2017

Member

@EPashkin Is this also what you meant? Could make the struct field public, and discuss names for the constructor, but apart from that there don't seem to be many other alternatives.

FromGlib can't be implemented properly here because we need the additional &str argument.

Member

sdroege commented May 18, 2017

@EPashkin Is this also what you meant? Could make the struct field public, and discuss names for the constructor, but apart from that there don't seem to be many other alternatives.

FromGlib can't be implemented properly here because we need the additional &str argument.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 18, 2017

Member

Yes I meant same, thanks
Also think that from_glib is not good.
Maybe rename it for check or try ?

We have time to think about naming and usage anyway,
I prefer apply gtk-rs/gir#371 and it effects first and it can take days.

Member

EPashkin commented May 18, 2017

Yes I meant same, thanks
Also think that from_glib is not good.
Maybe rename it for check or try ?

We have time to think about naming and usage anyway,
I prefer apply gtk-rs/gir#371 and it effects first and it can take days.

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 18, 2017

Member

check and try seem equally confusing. IMHO it should be some from_XXX for consistency with other API, as we create something from the arguments.

Member

sdroege commented May 18, 2017

check and try seem equally confusing. IMHO it should be some from_XXX for consistency with other API, as we create something from the arguments.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 18, 2017

Member

from IMHO also confusing as it don't produce BoolError.
Maybe to_result ? But by convention it not static member :(

Member

EPashkin commented May 18, 2017

from IMHO also confusing as it don't produce BoolError.
Maybe to_result ? But by convention it not static member :(

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 20, 2017

Member

@GuillaumeGomez that you think about this change?
If you accept, I merge gtk-rs/gir#373.
@sdroege When Gir's PR merged, please generate all *env that currently ignored

Member

EPashkin commented May 20, 2017

@GuillaumeGomez that you think about this change?
If you accept, I merge gtk-rs/gir#373.
@sdroege When Gir's PR merged, please generate all *env that currently ignored

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez May 20, 2017

Member

I have to admit that I don't see the point of having a BoolError type. It doesn't bring anything except for the function name. If you really want to not return a bool in case of failure, then let return Option<bool> or Result<bool, ()> directly.

If you think I missed something, I'm open to discussion so don't hesitate to bring the explanations. :p

Member

GuillaumeGomez commented May 20, 2017

I have to admit that I don't see the point of having a BoolError type. It doesn't bring anything except for the function name. If you really want to not return a bool in case of failure, then let return Option<bool> or Result<bool, ()> directly.

If you think I missed something, I'm open to discussion so don't hesitate to bring the explanations. :p

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 20, 2017

Member

@GuillaumeGomez Main reason possibility of use try, see gtk-rs/gir#368

Member

EPashkin commented May 20, 2017

@GuillaumeGomez Main reason possibility of use try, see gtk-rs/gir#368

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez May 20, 2017

Member

Then why create a whole new type? Just returning a Result<bool, ()> would be enough, no?

Member

GuillaumeGomez commented May 20, 2017

Then why create a whole new type? Just returning a Result<bool, ()> would be enough, no?

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 20, 2017

Member

Just returning Result<(), ()> would work, yes. But it seems quite ugly, instead of some actual error type.

Also it's not just the try macro, but also the compiler warning about unchecked results.

Member

sdroege commented May 20, 2017

Just returning Result<(), ()> would work, yes. But it seems quite ugly, instead of some actual error type.

Also it's not just the try macro, but also the compiler warning about unchecked results.

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 20, 2017

Member

Also with the function name in there you at least have some idea where it comes from when propagating the error further up and wrapping it in more meaningful error types

Member

sdroege commented May 20, 2017

Also with the function name in there you at least have some idea where it comes from when propagating the error further up and wrapping it in more meaningful error types

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 20, 2017

Member

() don't implement Error

Member

EPashkin commented May 20, 2017

() don't implement Error

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez May 20, 2017

Member

Ok, your arguments are valid so now I agree. Thanks a lot for this PR!

@EPashkin: I let you merge gir then I guess this code will need to be regenerated?

Member

GuillaumeGomez commented May 20, 2017

Ok, your arguments are valid so now I agree. Thanks a lot for this PR!

@EPashkin: I let you merge gir then I guess this code will need to be regenerated?

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin
Member

EPashkin commented May 20, 2017

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 21, 2017

Member

@sdroege When Gir's PR merged, please generate all *env that currently ignored

Please merge the first commit (adding the BoolError type) at the same time, otherwise GIR will generate code that can't compile. I removed the g_setenv() commit from this pull request for now.

I'll open a new pull request that updates g_setenv() and others after the other parts are merged.

For g_*env(), you'd like to have them autogenerated instead of manually implemented? I can do that, I was wondering anyway why you implemented them manually (especially because they don't work only on Unix-like OS, but work just fine on Windows too).

Member

sdroege commented May 21, 2017

@sdroege When Gir's PR merged, please generate all *env that currently ignored

Please merge the first commit (adding the BoolError type) at the same time, otherwise GIR will generate code that can't compile. I removed the g_setenv() commit from this pull request for now.

I'll open a new pull request that updates g_setenv() and others after the other parts are merged.

For g_*env(), you'd like to have them autogenerated instead of manually implemented? I can do that, I was wondering anyway why you implemented them manually (especially because they don't work only on Unix-like OS, but work just fine on Windows too).

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 21, 2017

Member

Just add commit here, with gir.toml, gir submodule and regen.

I get src/utils.rs:39: undefined reference tog_getenv'` when attempt of using it on Windows + MSYS2

Member

EPashkin commented May 21, 2017

Just add commit here, with gir.toml, gir submodule and regen.

I get src/utils.rs:39: undefined reference tog_getenv'` when attempt of using it on Windows + MSYS2

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 21, 2017

Member

I'll double check later, maybe it's a macro on Windows. I definitely used it on Windows from C code though and even the docs mention the Windows behaviour of it.

Member

sdroege commented May 21, 2017

I'll double check later, maybe it's a macro on Windows. I definitely used it on Windows from C code though and even the docs mention the Windows behaviour of it.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 21, 2017

Member

On windows it just named differently: g_getenv_utf8.
Code was in https://github.com/GNOME/glib/blob/master/glib/genviron.h but seems it removed this spring GNOME/glib@b67d071
So currently it better stays unix only.

Member

EPashkin commented May 21, 2017

On windows it just named differently: g_getenv_utf8.
Code was in https://github.com/GNOME/glib/blob/master/glib/genviron.h but seems it removed this spring GNOME/glib@b67d071
So currently it better stays unix only.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 21, 2017

Member

There many functions like that. We planed support naming difference sometime.
But if it replaced now situation worsened: we have old libraries that use "_utf8" suffix and new that don't 😢

Member

EPashkin commented May 21, 2017

There many functions like that. We planed support naming difference sometime.
But if it replaced now situation worsened: we have old libraries that use "_utf8" suffix and new that don't 😢

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 21, 2017

Member

I'll investigate at home, going through code on the phone is not very pleasant

Glib changing API is a problem in any case, will talk to them. Thanks for pointing that out

Member

sdroege commented May 21, 2017

I'll investigate at home, going through code on the phone is not very pleasant

Glib changing API is a problem in any case, will talk to them. Thanks for pointing that out

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 21, 2017

Member

@EPashkin You were misreading, it's all there still:
https://github.com/GNOME/glib/blob/master/glib/genviron.h#L36
https://github.com/GNOME/glib/blob/master/glib/genviron.c#L416

It's not just always going to the normal function and using UTF8 instead of the system's codepage. All good here

Member

sdroege commented May 21, 2017

@EPashkin You were misreading, it's all there still:
https://github.com/GNOME/glib/blob/master/glib/genviron.h#L36
https://github.com/GNOME/glib/blob/master/glib/genviron.c#L416

It's not just always going to the normal function and using UTF8 instead of the system's codepage. All good here

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 21, 2017

Member

So what should happen about this? For GLib < 2.53 there is g_getenv() for Unix-like OS, g_getenv_utf8() for Windows (and g_getenv() on Windows that returns strings in the system codepage, which we don't want). For GLib >= 2.53, there's only g_getenv() in the headers and it always gives UTF8 (e.g. is equivalent to g_getenv_utf8() on Windows), and g_getenv_utf8() still exists on Windows (just not in the headers).

Maybe for the time being the following: Always expose a manually implemented g_getenv(), which on Windows calls g_getenv_utf8() (which is always available for the time being, maybe just not in the headers) and g_getenv() elsewhere. And in some far away future, we just call g_getenv() always once glib-rs depends on GLib >= 2.53

Member

sdroege commented May 21, 2017

So what should happen about this? For GLib < 2.53 there is g_getenv() for Unix-like OS, g_getenv_utf8() for Windows (and g_getenv() on Windows that returns strings in the system codepage, which we don't want). For GLib >= 2.53, there's only g_getenv() in the headers and it always gives UTF8 (e.g. is equivalent to g_getenv_utf8() on Windows), and g_getenv_utf8() still exists on Windows (just not in the headers).

Maybe for the time being the following: Always expose a manually implemented g_getenv(), which on Windows calls g_getenv_utf8() (which is always available for the time being, maybe just not in the headers) and g_getenv() elsewhere. And in some far away future, we just call g_getenv() always once glib-rs depends on GLib >= 2.53

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 21, 2017

Member

Thanks for pointing that we still can use _utf8 even in new version.

Member

EPashkin commented May 21, 2017

Thanks for pointing that we still can use _utf8 even in new version.

sdroege added some commits May 21, 2017

Add Windows implementations for g_setenv, g_getenv, g_unsetenv and g_…
…get_current_dir

There are equivalent functions with the _utf8() prefix on Windows, to
which macros with the "normal" names are pointing.
@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 21, 2017

Member

This is now on top of #171

Member

sdroege commented May 21, 2017

This is now on top of #171

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 21, 2017

Member

Last commit IMHO unneeded as likely don't need regen

Member

EPashkin commented May 21, 2017

Last commit IMHO unneeded as likely don't need regen

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 21, 2017

Member

Yes, removed then :) FWIW, gtk::init() is another good candidate for BoolError.

Member

sdroege commented May 21, 2017

Yes, removed then :) FWIW, gtk::init() is another good candidate for BoolError.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 21, 2017

Member

@GuillaumeGomez CI passed, please do final check this PR

Member

EPashkin commented May 21, 2017

@GuillaumeGomez CI passed, please do final check this PR

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez May 21, 2017

Member

All good for me. Thanks!

Member

GuillaumeGomez commented May 21, 2017

All good for me. Thanks!

@GuillaumeGomez GuillaumeGomez merged commit 76cd600 into gtk-rs:master May 21, 2017

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