fix unicode issues and implement left truncation #285
Conversation
|
thanks |
|
Pushed new code that addresses both the existing unicode issues and adds left truncation. |
moh-eulith
changed the title
|
Hi @moh-eulith there is a lot of diff churn here, can you ensure that no rearranging happens? If you'd like to do some refactoring lets split that into a separate PR |
|
Diff was just trying too hard to make sense of a simple delete/add. Moved the add and now diff is a lot more readable. |
|
Is the diff ok now? |
|
Diff is looking good! |
src/encode/pattern/mod.rs
Outdated
| Style(Style), | ||
| } | ||
|
|
||
| struct StringBasedWriter<'a> { |
There was a problem hiding this comment.
nit: prefer usefully named lifetime params such as 'writer or 'params.
nit: although a single lifetime works here if someone uses those params separately they will need to refactor this struct to use separate lifetimes.
src/encode/pattern/mod.rs
Outdated
| } | ||
|
|
||
| enum StringOrStyle { | ||
| String(GraphemeLenString), |
There was a problem hiding this comment.
nit: suggest using struct-style variant
src/encode/pattern/mod.rs
Outdated
| params: &'a Parameters, | ||
| } | ||
|
|
||
| impl<'a> encode::Write for StringBasedWriter<'a> { |
There was a problem hiding this comment.
nit:
the lifetime(s) dont need to be named here
impl encode::Write for StringBasedWriter<'_, '_> {
| if let Ok(Some(x)) = r { | ||
| start = x; | ||
| } else { | ||
| // this should never happen, as we sanitize with to_utf8_lossy |
There was a problem hiding this comment.
Are we sure about this? Seems like it could, given any input of g and count if we rely on assumption that should be at least documented if not checked.
There was a problem hiding this comment.
We don't rely on that assumption. Clarified the comment.
|
Looks good other than above comments. |
|
addressed review comments and added a bit of documentation |
|
Is there anything else I can address for this PR? |
|
@moh-eulith Lets get this rebased with the latest as a start and we can take another look |
|
@bconn98 done. |
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #285 +/- ##
==========================================
+ Coverage 63.42% 64.12% +0.70%
==========================================
Files 25 25
Lines 1572 1586 +14
==========================================
+ Hits 997 1017 +20
+ Misses 575 569 -6 ☔ View full report in Codecov by Sentry. |
|
Is this ready for another look? |
Yes. |
See #283