Add VisualLines newtype wrapper#616
Add VisualLines newtype wrapper#616djc merged 3 commits intoconsole-rs:mainfrom smoelius:newtype-wrapper
VisualLines newtype wrapper#616Conversation
|
Thanks! First impressions are that I actually really like this. There are a lot fewer casts than I expected. (I also didn't know about The real question will be whether or not it makes the code clearer and easier to read though. I'm going to take some time to digest it over the next few days. Thanks again for the work! |
djc
left a comment
There was a problem hiding this comment.
LGTM modulo nits, thanks for working on this!
src/draw_target.rs
Outdated
| use std::time::Instant; | ||
|
|
||
| use console::Term; | ||
| use derive_more::{Add, AddAssign, Sub}; |
There was a problem hiding this comment.
I'd prefer to avoid this extra dependency in favor of just writing out the impls manually.
src/draw_target.rs
Outdated
| kind: TargetKind::Term { | ||
| term, | ||
| last_line_count: 0, | ||
| last_line_count: VisualLines::zero(), |
There was a problem hiding this comment.
I think I'd prefer relying on VisualLines::default() instead of zero() especially for cases like these where we're initializing. Is there any case where you think that's unreasonable and zero() makes for a better alternative?
There was a problem hiding this comment.
Is there any case where you think that's unreasonable and
zero()makes for a better alternative?
Not really. And I don't have a strong preference.
src/draw_target.rs
Outdated
| /// Adjust `last_line_count` such that the next draw operation keeps/clears additional lines | ||
| fn adjust_last_line_count(&mut self, adjust: LineAdjust) { | ||
| let last_line_count: &mut usize = match self { | ||
| let last_line_count: &mut VisualLines = match self { |
There was a problem hiding this comment.
Nit: let's just get rid of the type annotation here?
src/draw_target.rs
Outdated
|
|
||
| pub(crate) fn visual_line_count<I: SliceIndex<[String], Output = [String]>>( | ||
| &self, | ||
| range: I, |
There was a problem hiding this comment.
Style nit: prefer writing this as impl SliceIndex<[String], Output = [String]>, avoiding the need for an intermediate I variable.
src/draw_target.rs
Outdated
| impl<T: Into<usize>> From<T> for VisualLines { | ||
| fn from(value: T) -> Self { | ||
| Self(value.into()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Style nit: move this below the inherent impl, please.
src/draw_target.rs
Outdated
| pub(crate) fn as_usize(&self) -> usize { | ||
| self.0 | ||
| } | ||
| pub(crate) fn saturating_add(&self, other: Self) -> Self { | ||
| Self(self.0.saturating_add(other.0)) | ||
| } | ||
| pub(crate) fn saturating_sub(&self, other: Self) -> Self { | ||
| Self(self.0.saturating_sub(other.0)) | ||
| } | ||
| pub(crate) fn zero() -> Self { | ||
| Self(0) | ||
| } |
There was a problem hiding this comment.
Style nits: let's order these zero() (if we're keeping it), the saturating methods, then as_usize(), each separated by an empty line.
src/draw_target.rs
Outdated
| } | ||
|
|
||
| #[test] | ||
| fn visual_lines_default_is_zero() { |
There was a problem hiding this comment.
Please move this into the mod tests below.
|
@djc I think I addressed all of your comments. Please let me know if there are any I missed. |
An attempt to add a newtype wrapper, as discussed in #612 (comment) and #612 (comment).
Notes
VisualLinesbecause that was the term introduced in fix and renamereal_len#608, but I am not tied to that name.derive_more.TermLiketrait. It has both "height" and "width" characteristics, and it seemed weird to modify just one.