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

feat: use inner Display implementation #1097

Closed EdJoPaTo closed 2 months ago

EdJoPaTo commented 2 months ago

This allows for format!("{:>42}) formatting. See https://doc.rust-lang.org/std/fmt/struct.Formatter.html#method.pad


I am not very attached to this. It feels more correct to reuse the inner Display implementation but we can also drop this. Main reason for this PR was to move it out of #1007

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 94.2%. Comparing base (699c2d7) to head (b28e7c4). Report is 4 commits behind head on main.

:exclamation: Current head b28e7c4 differs from pull request most recent head d2dd6e4. Consider uploading reports for the commit d2dd6e4 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1097 +/- ## ===================================== Coverage 94.2% 94.2% ===================================== Files 61 61 Lines 14530 14533 +3 ===================================== + Hits 13693 13696 +3 Misses 837 837 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

joshka commented 2 months ago

Added tests, and switched to use formatter.pad instead of the calling display on the inner value. nit: This is somewhere where I'd generally feel fine using a single letter parameter name as that's idiomatic throughout the entire std lib whenever a debug / display impl is written.

EdJoPaTo commented 2 months ago

nit: This is somewhere where I'd generally feel fine using a single letter parameter name as that's idiomatic throughout the entire std lib whenever a debug / display impl is written.

Personally I use fmt but so far I also used core::fmt::… so I'm not sure how much the similar names confuses in such a case. I have clippy::min_ident_chars enabled which is annoyed at the single character here so I change it regularly. We can use f in ratatui, I disable that lint in ratatui anyway as its very annoying here.

joshka commented 2 months ago

nit: This is somewhere where I'd generally feel fine using a single letter parameter name as that's idiomatic throughout the entire std lib whenever a debug / display impl is written.

Personally I use fmt but so far I also used core::fmt::… so I'm not sure how much the similar names confuses in such a case. I have clippy::min_ident_chars enabled which is annoyed at the single character here so I change it regularly. We can use f in ratatui, I disable that lint in ratatui anyway as its very annoying here.

It's not worth changing this back, noting it as a not worth changing for future PRs.

EdJoPaTo commented 2 months ago

as that's idiomatic throughout the entire std lib

They are also not that aligned with more speaking variable / function names and like to abbreviate way more often than I like. I remember being annoyed by the lack of clear names in the std lib when I started with Rust as that was more confusing than necessary in my opinion. After all, most people use language servers with completion.

joshka commented 2 months ago

as that's idiomatic throughout the entire std lib

They are also not that aligned with more speaking variable / function names and like to abbreviate way more often than I like. I remember being annoyed by the lack of clear names in the std lib when I started with Rust as that was more confusing than necessary in my opinion. After all, most people use language servers with completion.

I'm also a "nvr abbrv." opinion holder generally, so I see your point on this. I always rename f to frame whenever I see it in ratatui code, because it poorly communicates what the value is for readers. When faced with choosing idiom vs opinion, I tend towards accepting the former most of the time.