Closed dkaste closed 5 years ago
Thanks @dkaste!
I like it, but yeah, this is possibly going to break a lot of people :-(. Would you be against keeping the old values as re-exports?
E.g.:
impl Color {
pub const RED: Color = Color{r: 255, g: 0, b: 0};
}
pub const RED: Color = Color::RED;
That way we keep compiling and we can add proper deprecation messages to the older way later.
(by the way, the Travis failure should be resolved with https://github.com/tomassedovic/tcod-rs/pull/292 so if you rebase, it should pass)
It definitely makes sense to keep the original constants for compatibility. I added them back in my PR and it should be ready to merge once CI finishes.
Ah, I just realized that you suggested to add them as re-exports. I'll fix it in a second.
Yeah, I mean you can keep the RGB values in as well, but I've got a slight preference to make it DRYer with reexports.
I hoped we could do something like this even:
pub use Color::RED;
(instead of pub const RED: Color = Color::RED;
)
But looks like associated constants can't be reexported with use
, sadly.
Alright, I've updated the PR so there's no repetition of actual data. I'm assuming if it's eventually planned to deprecate (and maybe remove) the free constants that it makes more sense to keep the color information in the associated constants.
Thanks!
Associated constants have been stable in Rust since 1.20. I think
tcod::Color::RED
is nicer thantcod::colors::RED
, though I understand this is down to personal preference. Maybe it's not even worth a breaking change, but I thought I would make the suggestion anyway. :smile: