search

ratatui-org / ratatui

Rust library that's all about cooking up terminal user interfaces (TUIs) 👨‍🍳🐀
https://ratatui.rs
MIT License
8.96k stars 274 forks source link

Reuse std::fmt::Alignment #1096

Closed EdJoPaTo closed 2 months ago

EdJoPaTo commented 2 months ago

Funny find while tooking into the core::fmt::Formatter. Just to pitch this as an idea:

We create our custom Alignment enum. core::fmt has an existing one. Reusing it for our formatting seems neat. Can also be reexported.

It has no Default but we use Option<Alignment> anyway.

joshka commented 2 months ago

What problem does it solve? What problems does it cause (e.g. existing apps)? What would this prevent in the future (maybe adding other alignments / adding methods for alignment related stuff to the impl block)?

Seems like if there's no problem to solve (or only very low impact ones), and there's a possible chance of a problem (however unlikely), it might be worth keeping our current code instead on this.

EdJoPaTo commented 2 months ago

unicode-truncate has its own Alignment definition which is incompatible with the one of ratatui. As long as stuff is reusable, why not reuse it?

When other stuff is needed in the future it can still be adapted and an own type introduced. Personally I prefer smaller breaking changes in comparison to over generic code that might be good for some future use that might never arise.

Its not really significant to use it, but it's still the same intention reinvented. Which is why it might be interesting to think about it.

It might be a good idea in the future to search the std lib for existing types before inventing the wheel again. Until the usage diverges enough to diverge.

joshka commented 2 months ago

Right now this works fine, and there's no huge need to change this. Let's try to focus on things that add value rather than looking to change every little thing.

Not sure if you're a sports fan at all, but often when a referee's call is being reviewed (e.g. a coach's challenge of a goal in hockey), the standard of review is that it was "clearly not correct". It could be worth applying some similar thinking to ideas for improvement like this. Having our own Alignment type works fine, it solves the problem and doesn't have any obvious bugs. Changing it won't help any apps do anything easier / better.

So while this is probably "technically correct", it's also probably a distraction not worth investing time into. Does that make sense?