PistonDevelopers / gfx_text

Draw text for gfx using freetype
http://docs.piston.rs/gfx_text/gfx_text/
MIT License
22 stars 12 forks source link

Idiomatic update to gfx-0.5 #18

Closed kvark closed 9 years ago

Kagami commented 9 years ago

Cool, thanks!

Little nits:

kvark commented 9 years ago

Glob imports is not generally a bad practice. It should be used for traits, which methods need to be visible to be called, not for types. gfx::traits::* is precisely this, except for your code tends to re-use types from there directly instead of typing the full path (e.g. gfx::Factory).

I do agree that spawning a factory for each text renderer would look better.

Kagami commented 9 years ago

It should be used for traits

What's the difference between function, struct and trait here? Glob imports are bad because:

In my opinion there is no excuse for glob imports except, maybe, reexporting or test code.

kvark commented 9 years ago

Looking at the guidelines:

Avoid use *, except in tests Prefer fully importing types/traits while module-qualifying functions.

It does seem to support your view, in general at least. However, use gfx::traits::* is a case that might be a good exception from this general rule, since it's importing traits only.

I don't think this is a good link for the guidelines though, it seems WIP and rough. The final version may be more descriptive.

Kagami commented 9 years ago

I just don't understand what's the profit of using glob here, given the arguments above. Just to type a few chars less?..

kvark commented 9 years ago

The profit is - we might be changing the exported traits under the hood in gfx-rs without breaking the API. Like, some of the methods from Canvas were moved to Stream, which Canvas implemented, and no one needed to fix any breaks.

kvark commented 9 years ago

Created https://github.com/gfx-rs/gfx-rs/issues/744

Kagami commented 9 years ago

The profit is - we might be changing the exported traits under the hood in gfx-rs without breaking the API

Well, the problem is, it can break someone's code as well. This won't compile:

mod a {
    pub trait Test {}
}

mod b {
    use a::*;
    trait Test {}
}
kvark commented 9 years ago

@Kagami should be good now

Kagami commented 9 years ago

Thanks!