Add VisualLines newtype wrapper
#616
Conversation
|
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! |
src/draw_target.rs
Outdated
| use std::sync::{Arc, RwLock, RwLockWriteGuard}; | ||
| use std::thread::panicking; | ||
| use std::time::Duration; | ||
| #[cfg(not(target_arch = "wasm32"))] | ||
| 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
| @@ -72,7 +74,7 @@ impl ProgressDrawTarget { | |||
| Self { | |||
| 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
| @@ -248,7 +250,7 @@ enum TargetKind { | |||
| impl TargetKind { | |||
| /// 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.
There was a problem hiding this comment.
Removed zero per the above request.
|
@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.