-
Notifications
You must be signed in to change notification settings - Fork 257
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
fix(buffer): add try methods #1112
fix(buffer): add try methods #1112
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1112 +/- ##
======================================
Coverage 94.3% 94.3%
======================================
Files 61 61
Lines 14768 14910 +142
======================================
+ Hits 13932 14070 +138
- Misses 836 840 +4 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an existing PR that covers much the same as this already:
#1084
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove
/// # Panics | ||
/// Panics if the coordinates are out of bounds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// # Panics | |
/// Panics if the coordinates are out of bounds | |
/// | |
/// # Panics | |
/// | |
/// Panics if the coordinates are out of bounds |
/// Returns an option containing a mutable reference to Cell at the given coordinates, or | ||
/// `None`, if the coordinates are out of bounds | ||
pub fn try_get_mut(&mut self, x: u16, y: u16) -> Option<&mut Cell> { | ||
let i = self.index_of(x, y); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will panic
/// assert_eq!(buffer.try_index_of(200, 100), Some(0)); | ||
/// ``` | ||
#[must_use] | ||
pub fn try_index_of(&self, x: u16, y: u16) -> Option<usize> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer not to add more cruft. The fact that the underlying storage of the cells is a Vec is an implementation detail, not something that should generally be relied on by callers. index_of
should be pub(crate) as should anything in buffer that deals directly with the idea of the index. That's a bigger change than this however. But, adding another public method makes that more annoying.
) | ||
} | ||
|
||
/// Print a string, starting at the position (x, y) | ||
/// # Panics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// # Panics | |
/// | |
/// # Panics |
and elsewhere
/// `style` accepts any type that is convertible to [`Style`] (e.g. [`Style`], [`Color`], or | ||
/// your own type that implements [`Into<Style>`]). | ||
#[must_use] | ||
pub fn try_set_string<T, S>(&mut self, x: u16, y: u16, string: T, style: S) -> Option<()> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to eventually deprecate these methods and use Line/Span render instead.
#[track_caller] | ||
pub fn get(&self, x: u16, y: u16) -> &Cell { | ||
let i = self.index_of(x, y); | ||
&self.content[i] | ||
} | ||
|
||
/// Returns an option containing a reference to Cell at the given coordinates, or `None`, if the | ||
/// coordinates are out of bounds | ||
pub fn try_get(&self, x: u16, y: u16) -> Option<&Cell> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be called get_opt as try_
indicates a method will return Result
. We briefly discussed naming in the underlying issue.
This PR partially resolves issue #1011