search

hannobraun / rustecs

Entity Component System for Rust
20 stars 5 forks source link

Allow full paths in `world!`'s component field #7

Closed s1gtrap closed 9 years ago

s1gtrap commented 9 years ago

This allows the use of full types when generating component code, such as

world! {
    components Position<f64>;
}

or

world! {
    components some::crate::Position;
}

while retaining the same general usage syntax as before. I haven't considered collisions, though I believe potential error messages would be quite clear.

jakerr commented 9 years ago

This looks good to me! What happens with a struct which has a lifetime parameter?

You might want to add a test with all the cases you intend to cover here https://github.com/hannobraun/rustecs/tree/master/rustecs/tests there are already lots of example tests to draw from :)

hannobraun commented 9 years ago

Looks good to me, too. Thanks a lot, @bheart!

But as @jakerr suggested, I'd like to have a test for this. Maybe a new, self-contained test file for components (tests/components.rs) would be the right place for this. I was thinking of adding something like this anyway. Just adding a world! declaration to that file to make sure it compiles should be enough, I think.

Other than that, this is good to merge!

s1gtrap commented 9 years ago

I would've liked to check if the functions generated properly (i.e. check name/type at runtime within the test), but I don't know how I'd go about that.

@jakerr I haven't thought of this, but provided the path is a valid type it shouldn't be a problem. I'll add it to the test.

s1gtrap commented 9 years ago

Thing is that that is the exact thing entities.rs tests for - the two cases are almost identical. Should I just throw some extra types in there then?

hannobraun commented 9 years ago

I think it's more than enough to throw some paths in a world! declaration somewhere. If it compiles, we can assume that it will works the same as the non-path types that test cases already exist for. If path-specific bugs arise in the future, we can add additional tests as we fix those, but I don't think it's worth the effort to think too much about it now :)

Add it to entities.rs or create a new components.rs. Both would be fine.

hannobraun commented 9 years ago

Excellent, merged. Thanks again, @bheart!

Sorry that it took me a while to see your test commit. GitHub doesn't seem to notify anyone when commits are pushed.