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

feat: use inner Display implementation #1097

Merged
merged 6 commits into from
May 13, 2024

Conversation

EdJoPaTo
Copy link
Contributor

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

Copy link

codecov bot commented May 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

❗ 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
@@          Coverage Diff          @@
##            main   #1097   +/-   ##
=====================================
  Coverage   94.2%   94.2%           
=====================================
  Files         61      61           
  Lines      14530   14533    +3     
=====================================
+ Hits       13693   13696    +3     
  Misses       837     837           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@EdJoPaTo EdJoPaTo force-pushed the reuse-inner-fmt-impl branch from 3dc256f to 5141cba Compare May 12, 2024 14:43
@joshka
Copy link
Member

joshka commented May 12, 2024

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

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

joshka commented May 12, 2024

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.

@joshka joshka merged commit eb281df into ratatui:main May 13, 2024
3 checks passed
@EdJoPaTo
Copy link
Contributor Author

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.

@EdJoPaTo EdJoPaTo deleted the reuse-inner-fmt-impl branch May 13, 2024 08:37
@joshka
Copy link
Member

joshka commented May 14, 2024

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.

joshka pushed a commit to nowNick/ratatui that referenced this pull request May 24, 2024
joshka pushed a commit to erak/ratatui that referenced this pull request Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants