Add padding width #1672
Add padding width #1672
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1672 +/- ##
==========================================
+ Coverage 91.06% 91.13% +0.06%
==========================================
Files 37 37
Lines 17454 17565 +111
==========================================
+ Hits 15895 16007 +112
+ Misses 1559 1558 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
BigPapa314
force-pushed
the
add-padding-width
branch
from
2df27ad to
e2cd821
Compare
BigPapa314
force-pushed
the
add-padding-width
branch
from
e2cd821 to
fd4e817
Compare
BigPapa314
force-pushed
the
add-padding-width
branch
from
fd4e817 to
3501de7
Compare
BigPapa314
force-pushed
the
add-padding-width
branch
from
3501de7 to
fe66485
Compare
BigPapa314
force-pushed
the
add-padding-width
branch
from
fe66485 to
4a89800
Compare
BigPapa314
force-pushed
the
add-padding-width
branch
from
4a89800 to
d89f8f9
Compare
|
Hmm the codecov/project task seams to be stuck. Can someone have a look? |
| #[cfg(not(feature = "alloc"))] | ||
| impl Numeric { | ||
| /// Adds the with of the padding to the numeric | ||
| /// | ||
| /// Should be removed if the padding width is added to the `Pad` enum. | ||
| pub fn with_padding(self, _width: usize) -> Self { | ||
| self | ||
| } | ||
|
|
||
| /// Gets the numeric and padding width from the numeric | ||
| /// | ||
| /// Should be removed if the padding width is added to the `Pad` enum. | ||
| pub fn unwrap_padding(&self) -> (&Self, usize) { | ||
| (self, 0) | ||
| } | ||
| } |
There was a problem hiding this comment.
Why is this useful? I don't think these should be part of the public API.
There was a problem hiding this comment.
Becauase we don't add the padding width to the Pad enum and try to add it using dynamic memory allocation to the Numeric enum we have multiple cases to handle if we want to work with padding. Because the lib can also be used without dynamic memory allocation the code has to be deactivated in that cases. This is done by the "with_padding" and "unwrap_padding" functions. Because the Numeric enum is public this functions should also be public to help working with the padding.
| (0, Pad::None) => {} | ||
| (0, Pad::Space) => w.write_char(' ')?, | ||
| (tens, _) => w.write_char((b'0' + tens) as char)?, | ||
| // unpack padding width if provided |
There was a problem hiding this comment.
I want your diff to have minimal changes to the structure of the code, or to isolate structural (but non-functional) changes in a separate commit. Is that feasible? The current iteration seems pretty hard to review.
There was a problem hiding this comment.
The original code only had the oportunity to have exactly one or two padding signs. So there where special functions "write_one" and "write_two" to exactly do that. Because we now have a dynamic amount of padding signs it whould make the code more complex to have some kind of selection logic for "write_xxx" methods when we already had a method to write any amount of padding signs.
|
In general this PR should be a proposal how it whould look like if we add the padding width to the Numeric enum and not to the Pad enum where it whould belong to. My conclusion is: Advantages:
Drawbacks:
I think we should postpone the changes untill we can add the padding with to where it should be so that we can implement it in a minimal invasive and maximal compatible way. |
Okay, but I have no concrete plans nor much motivation to work on chrono 0.5, so that might mean postponing more or less indefinitely. |
Fixes #1663