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

Consider removing the Lua prefix from types/traits #15

Closed
jonas-schievink opened this issue Jul 16, 2017 · 3 comments
Closed

Consider removing the Lua prefix from types/traits #15

jonas-schievink opened this issue Jul 16, 2017 · 3 comments

Comments

@jonas-schievink
Copy link
Contributor

The rlua crate is already a namespace for the contents, so they don't need a prefix for disambiguation. Removing the prefix would allow writing shorter and slightly clearer code in my opinion.

Downside: use rlua::*; imports too much (including a Result type alias shadowing Rust's own result type). This can be mitigated somewhat by introducing a prelude module like many libs do. It's also not clear to me if renaming the LuaString type to String is a good idea.

(also, LightUserData is the only type that's not prefixed)

@jonas-schievink
Copy link
Contributor Author

While we're at it, the trait I have to implement to make something a userdata object is named LuaUserDataType, which is really long. We could rename it to UserData, and the current LuaUserData could be AnyUserData or UserDataAny to make clear that it's doing runtime type checking like std::any::Any.

@Timidger
Copy link
Contributor

Personally, I think it's fine to name things somewhat generically in Rust since they are namespaced (e.g String) however you might need to do some refactoring in places (e.g a function that returns a std::string::String would not compile if rlua::String is in scope).

You shouldn't really be doing use <lib>::* except in extreme cases IMHO, it makes it much harder for readers to know where types come from (especially because IDE support for Rust is lacking right now) and it can introduce problems like this. So I wouldn't worry about the potential downside that this makes a glob import unusable.

@kyren
Copy link
Contributor

kyren commented Jul 24, 2017

PR has been merged, so I'm going to mark this issue as fixed. I added a 'prelude' module for glob imports, but I don't know whether it's "really" worth it or not, I guess it's something to consider before a hypothetical 1.0 release. I will use the prelude module though, so it's worth it for me at least.

@kyren kyren closed this as completed Jul 24, 2017
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

No branches or pull requests

3 participants