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

[Proposal] Change conversion functions to conform to Rust's convention #1036

Open
danielrempel opened this issue Oct 16, 2020 · 2 comments · May be fixed by #1037
Open

[Proposal] Change conversion functions to conform to Rust's convention #1036

danielrempel opened this issue Oct 16, 2020 · 2 comments · May be fixed by #1037

Comments

@danielrempel
Copy link

Got used to Rust's naming conventions and among them there's a less official one that recommends to name 'free' type casting functions as 'as_something()' and expensive ones as 'to_something()'

Surface::as_texture() actually creates a new texture and copies surface data into it, so I believe it should be named to_texture()

Grep'ed the code and found several more cases like:

./src/sdl2/video.rs:98:    fn to_gl_value(self) -> i32 { self as i32 }

It is problematic to change the APIs as there must be a lot of projects building on top of rust-sdl2, but maybe as_texture() and others could be declared obsolete and removed when version bump allows that?

Rust supports deprecation marks

Semver 2.0 requires a minor version bump to deprecate and then a major bump to remove deprecated methods

I could do the changes myself if you agree that they are reasonable / worthy :)

@Cobrand
Copy link
Member

Cobrand commented Oct 16, 2020

Sure, sounds good to me!

You could rename the old to the new functions, but still keep the old ones as an alias to the new ones, and mark them as deprecated. After a few versions, we could remove them entirely and get rid of the deprecation markers.

@Lokathor
Copy link

Well the important reason why as_foo is cheap is because it's supposed to be reference to reference, and then to_foo is potentially expensive because it's owned to owned.

So Surface -> Texture should be to_texture but GL data type casting should also be to_gl_value

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.

3 participants