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

Rename get_ methods #963

Closed
Tracked by #137
anyputer opened this issue Aug 11, 2020 · 11 comments · Fixed by #1021
Closed
Tracked by #137

Rename get_ methods #963

anyputer opened this issue Aug 11, 2020 · 11 comments · Fixed by #1021

Comments

@anyputer
Copy link

The get_ methods should be renamed to fit the amazing Rust API Guidelines.

This would apply to the other crates as well.
Yes, there are a lot of methods, but it would lead to nicer, more rusty code.

Examples

ButtonExt.get_text() -> ButtonExt.text()
LabelExt.get_angle() -> LabelExt.angle()

Perhaps methods like the following could look different?
ButtonExt.get_always_show_image() -> ButtonExt.always_shows_image()
ButtonExt.get_use_underline() ->ButtonExt.uses_underline()
Ehh...probably not.... it's just something to consider.

@GuillaumeGomez
Copy link
Member

I personally think we should try to stick to a mix between rust API guidelines and providing functions with name close to the C ones (to make the search simpler).

@sdroege
Copy link
Member

sdroege commented Aug 12, 2020

I think this could make sense. We'd have to try with the code generator if that ends up with a nice API or is just confusing in the end, or if there are even conflicts between functions. And we already have this naming scheme in some of the manual API in glib.

Also should consider converting set_property_XXX and get_property_XXX to not include the "property" part at the same time. Basically the same thing, and other bindings also don't include the information that it's a property. If there's also a proper setter/getter then there's just no code generated for using the properties.

@EPashkin What do you think?

@EPashkin
Copy link
Member

I agree that we can remove "property" for properties,
Not sure about "get" prefixes: agree with @GuillaumeGomez that we better have same names as C counterparts, but maybe we can make exception for getters.

@sdroege sdroege transferred this issue from gtk-rs/gtk Sep 14, 2020
@sdroege
Copy link
Member

sdroege commented Nov 24, 2020

So @fengalin created https://gitlab.freedesktop.org/gstreamer/gstreamer-rs/-/merge_requests/639 and I think this improves the API a lot, making it more Rust-y.

I think we should do the same here for gtk-rs: most getters should have their get_ prefix removed, very few should keep it and some need special handling: get_is_foo -> is_foo, and others where the second word is a verb. We can implement heuristics for that and the 5 cases not covered by one of the heuristics can be manually fixed via Gir.toml.

And additionally, we already have many getters in manual code that don't have the get_ prefix.

@bilelmoussaoui @GuillaumeGomez What do you think?

@sdroege
Copy link
Member

sdroege commented Nov 24, 2020

Also this should be done together with #962

@bilelmoussaoui
Copy link
Member

bilelmoussaoui commented Nov 24, 2020

I totally agree on this on renaming both cases. Other language bindings do make the properties usage seamless, for example Python can lets you modify the properties using object.props.some_prop = value, the same +/- for GJS & Vala. It would be nice if we can do all the renaming changes once though to avoid having to re-update everything multiple times.

See #936 #974 #905

@fengalin
Copy link
Contributor

fengalin commented Nov 24, 2020

Based on my experience of doing that manually in gstreamer-rs, bedsides implementing this in gir, we also need an automatic process for manual code conversion. I'm not skilled at batch text processing at all, but I think that goes beyond its capabilities. If we all agree on this, I'll take a look at implementing a parser / filter for this where we could setup the heuristics. Maybe we could also fix the call places in the process. Hopefully, this parser / filter could be useful for other future changes.

Edit: @GuillaumeGomez, I followed your advice where possible :) https://gitlab.freedesktop.org/fengalin/gstreamer-rs/-/blob/remove-get_-for-getters/gstreamer/src/buffer.rs#L305

@sdroege
Copy link
Member

sdroege commented Nov 24, 2020

we also need an automatic process for manual code conversion

What do you mean with that btw? I would've assumed that sed -i 's;fn get_;fn ;g' **.rs would done the majority of work for the manual code already, and then we're only left with the special cases.

@fengalin
Copy link
Contributor

What do you mean with that btw?

For tracing: see my response to your other comment there.

@GuillaumeGomez
Copy link
Member

@fengalin You're my hero! \o/

And if that doesn't create conflicts, I completely approve.

@sdroege
Copy link
Member

sdroege commented Nov 25, 2020

This is explicitly mentioned in the Rust style guidelines btw: https://doc.rust-lang.org/1.0.0/style/style/naming/README.html#getter/setter-methods-[rfc-344] .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants