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

Reuse std::fmt::Alignment #1096

Closed
EdJoPaTo opened this issue May 12, 2024 · 3 comments
Closed

Reuse std::fmt::Alignment #1096

EdJoPaTo opened this issue May 12, 2024 · 3 comments
Labels
enhancement New feature or request

Comments

@EdJoPaTo
Copy link
Member

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.

@EdJoPaTo EdJoPaTo added the enhancement New feature or request label May 12, 2024
@joshka
Copy link
Member

joshka commented May 12, 2024

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
Copy link
Member Author

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
Copy link
Member

joshka commented May 12, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants