Skip to content

Add padding width #1672

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add padding width #1672

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 45 additions & 76 deletions src/format/formatting.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,89 +127,58 @@
fn format_numeric(&self, w: &mut impl Write, spec: &Numeric, pad: Pad) -> fmt::Result {
use self::Numeric::*;

fn write_one(w: &mut impl Write, v: u8) -> fmt::Result {
w.write_char((b'0' + v) as char)
}

fn write_two(w: &mut impl Write, v: u8, pad: Pad) -> fmt::Result {
let ones = b'0' + v % 10;
match (v / 10, pad) {
(0, Pad::None) => {}
(0, Pad::Space) => w.write_char(' ')?,
(tens, _) => w.write_char((b'0' + tens) as char)?,
// unpack padding width if provided
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let (spec, mut pad_width) = spec.unwrap_padding();

let (value, default_pad_width, is_year) = match (spec, self.date, self.time) {
(Year, Some(d), _) => (d.year() as i64, 4, true),
(YearDiv100, Some(d), _) => (d.year().div_euclid(100) as i64, 2, false),
(YearMod100, Some(d), _) => (d.year().rem_euclid(100) as i64, 2, false),
(IsoYear, Some(d), _) => (d.iso_week().year() as i64, 4, true),
(IsoYearDiv100, Some(d), _) => (d.iso_week().year().div_euclid(100) as i64, 2, false),

Check warning on line 138 in src/format/formatting.rs

View check run for this annotation

Codecov / codecov/patch

src/format/formatting.rs#L138 

Added line #L138 was not covered by tests
(IsoYearMod100, Some(d), _) => (d.iso_week().year().rem_euclid(100) as i64, 2, false),
(Quarter, Some(d), _) => (d.quarter() as i64, 1, false),
(Month, Some(d), _) => (d.month() as i64, 2, false),
(Day, Some(d), _) => (d.day() as i64, 2, false),
(WeekFromSun, Some(d), _) => (d.weeks_from(Weekday::Sun) as i64, 2, false),
(WeekFromMon, Some(d), _) => (d.weeks_from(Weekday::Mon) as i64, 2, false),
(IsoWeek, Some(d), _) => (d.iso_week().week() as i64, 2, false),
(NumDaysFromSun, Some(d), _) => (d.weekday().num_days_from_sunday() as i64, 1, false),
(WeekdayFromMon, Some(d), _) => (d.weekday().number_from_monday() as i64, 1, false),
(Ordinal, Some(d), _) => (d.ordinal() as i64, 3, false),
(Hour, _, Some(t)) => (t.hour() as i64, 2, false),
(Hour12, _, Some(t)) => (t.hour12().1 as i64, 2, false),
(Minute, _, Some(t)) => (t.minute() as i64, 2, false),
(Second, _, Some(t)) => {
((t.second() + t.nanosecond() / 1_000_000_000) as i64, 2, false)
}
w.write_char(ones as char)
}

#[inline]
fn write_year(w: &mut impl Write, year: i32, pad: Pad) -> fmt::Result {
if (1000..=9999).contains(&year) {
// fast path
write_hundreds(w, (year / 100) as u8)?;
write_hundreds(w, (year % 100) as u8)
} else {
write_n(w, 4, year as i64, pad, !(0..10_000).contains(&year))
(Nanosecond, _, Some(t)) => ((t.nanosecond() % 1_000_000_000) as i64, 9, false),
(Timestamp, Some(d), Some(t)) => {
let offset = self.off.as_ref().map(|(_, o)| i64::from(o.local_minus_utc()));
(d.and_time(t).and_utc().timestamp() - offset.unwrap_or(0), 9, false)
}
}
(Internal(_), _, _) => return Ok(()), // for future expansion
(Padded { .. }, _, _) => return Err(fmt::Error), // should be unwrapped above
_ => return Err(fmt::Error), // insufficient arguments for given format

Check warning on line 162 in src/format/formatting.rs

View check run for this annotation

Codecov / codecov/patch

src/format/formatting.rs#L160-L162 

Added lines #L160 - L162 were not covered by tests
};

fn write_n(
w: &mut impl Write,
n: usize,
v: i64,
pad: Pad,
always_sign: bool,
) -> fmt::Result {
if always_sign {
match pad {
Pad::None => write!(w, "{:+}", v),
Pad::Zero => write!(w, "{:+01$}", v, n + 1),
Pad::Space => write!(w, "{:+1$}", v, n + 1),
}
} else {
match pad {
Pad::None => write!(w, "{}", v),
Pad::Zero => write!(w, "{:01$}", v, n),
Pad::Space => write!(w, "{:1$}", v, n),
}
}
if pad_width == 0 {
pad_width = default_pad_width;
}

match (spec, self.date, self.time) {
(Year, Some(d), _) => write_year(w, d.year(), pad),
(YearDiv100, Some(d), _) => write_two(w, d.year().div_euclid(100) as u8, pad),
(YearMod100, Some(d), _) => write_two(w, d.year().rem_euclid(100) as u8, pad),
(IsoYear, Some(d), _) => write_year(w, d.iso_week().year(), pad),
(IsoYearDiv100, Some(d), _) => {
write_two(w, d.iso_week().year().div_euclid(100) as u8, pad)
}
(IsoYearMod100, Some(d), _) => {
write_two(w, d.iso_week().year().rem_euclid(100) as u8, pad)
}
(Quarter, Some(d), _) => write_one(w, d.quarter() as u8),
(Month, Some(d), _) => write_two(w, d.month() as u8, pad),
(Day, Some(d), _) => write_two(w, d.day() as u8, pad),
(WeekFromSun, Some(d), _) => write_two(w, d.weeks_from(Weekday::Sun) as u8, pad),
(WeekFromMon, Some(d), _) => write_two(w, d.weeks_from(Weekday::Mon) as u8, pad),
(IsoWeek, Some(d), _) => write_two(w, d.iso_week().week() as u8, pad),
(NumDaysFromSun, Some(d), _) => write_one(w, d.weekday().num_days_from_sunday() as u8),
(WeekdayFromMon, Some(d), _) => write_one(w, d.weekday().number_from_monday() as u8),
(Ordinal, Some(d), _) => write_n(w, 3, d.ordinal() as i64, pad, false),
(Hour, _, Some(t)) => write_two(w, t.hour() as u8, pad),
(Hour12, _, Some(t)) => write_two(w, t.hour12().1 as u8, pad),
(Minute, _, Some(t)) => write_two(w, t.minute() as u8, pad),
(Second, _, Some(t)) => {
write_two(w, (t.second() + t.nanosecond() / 1_000_000_000) as u8, pad)
}
(Nanosecond, _, Some(t)) => {
write_n(w, 9, (t.nanosecond() % 1_000_000_000) as i64, pad, false)
let always_sign = is_year && !(0..10_000).contains(&value);
if always_sign {
match pad {
Pad::None => write!(w, "{:+}", value),

Check warning on line 172 in src/format/formatting.rs

View check run for this annotation

Codecov / codecov/patch

src/format/formatting.rs#L172 

Added line #L172 was not covered by tests
Pad::Zero => write!(w, "{:+01$}", value, pad_width + 1),
Pad::Space => write!(w, "{:+1$}", value, pad_width + 1),

Check warning on line 174 in src/format/formatting.rs

View check run for this annotation

Codecov / codecov/patch

src/format/formatting.rs#L174 

Added line #L174 was not covered by tests
}
(Timestamp, Some(d), Some(t)) => {
let offset = self.off.as_ref().map(|(_, o)| i64::from(o.local_minus_utc()));
let timestamp = d.and_time(t).and_utc().timestamp() - offset.unwrap_or(0);
write_n(w, 9, timestamp, pad, false)
} else {
match pad {
Pad::None => write!(w, "{}", value),
Pad::Zero => write!(w, "{:01$}", value, pad_width),
Pad::Space => write!(w, "{:1$}", value, pad_width),
}
(Internal(_), _, _) => Ok(()), // for future expansion
_ => Err(fmt::Error), // insufficient arguments for given format
}
}

Expand Down
103 changes: 103 additions & 0 deletions src/format/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@

#[cfg(all(feature = "alloc", not(feature = "std"), not(test)))]
use alloc::boxed::Box;
#[cfg(feature = "alloc")]
use alloc::sync::Arc;
use core::fmt;
use core::str::FromStr;
#[cfg(feature = "std")]
Expand Down Expand Up @@ -149,13 +151,71 @@ pub enum Numeric {
/// For formatting, it assumes UTC upon the absence of time zone offset.
Timestamp,

#[cfg(feature = "alloc")]
/// An extension to carry the width of the padding.
Padded {
/// The numeric to be padded.
numeric: Arc<Numeric>,
/// The width of the padding.
width: usize,
},

/// Internal uses only.
///
/// This item exists so that one can add additional internal-only formatting
/// without breaking major compatibility (as enum variants cannot be selectively private).
Internal(InternalNumeric),
}

#[cfg(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 {
if width != 0 {
// update padding
match self {
Numeric::Padded { numeric, .. } => Numeric::Padded { numeric, width },
numeric => Numeric::Padded { numeric: Arc::new(numeric), width },
}
} else {
// remove padding
match self {
Numeric::Padded { numeric, .. } => numeric.as_ref().clone(),
numeric => numeric,
}
}
}

/// 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) {
match self {
Numeric::Padded { numeric, width } => (numeric.as_ref(), *width),
numeric => (numeric, 0),
}
}
}

#[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)
}
}
Comment on lines +202 to +217
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this useful? I don't think these should be part of the public API.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/// An opaque type representing numeric item types for internal uses only.
#[derive(Clone, Eq, Hash, PartialEq)]
pub struct InternalNumeric {
Expand Down Expand Up @@ -555,3 +615,46 @@ impl FromStr for Month {
}
}
}

#[cfg(test)]
mod tests {
use crate::format::*;

#[test]
#[cfg(feature = "alloc")]
fn test_numeric_with_padding() {
// No padding
assert_eq!(Numeric::Year.with_padding(0), Numeric::Year);

// Add padding
assert_eq!(
Numeric::Year.with_padding(5),
Numeric::Padded { numeric: Arc::new(Numeric::Year), width: 5 }
);

// Update padding
assert_eq!(
Numeric::Year.with_padding(5).with_padding(10),
Numeric::Padded { numeric: Arc::new(Numeric::Year), width: 10 }
);

// Remove padding
assert_eq!(Numeric::Year.with_padding(5).with_padding(0), Numeric::Year);
}

#[test]
#[cfg(not(feature = "alloc"))]
fn test_numeric_with_padding_disabled() {
// No padding
assert_eq!(Numeric::Year.with_padding(0), Numeric::Year);

// Add padding
assert_eq!(Numeric::Year.with_padding(5), Numeric::Year);

// Update padding
assert_eq!(Numeric::Year.with_padding(5).with_padding(10), Numeric::Year);

// Remove padding
assert_eq!(Numeric::Year.with_padding(5).with_padding(0), Numeric::Year);
}
}
88 changes: 87 additions & 1 deletion src/format/parse.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
use core::borrow::Borrow;
use core::str;

#[cfg(feature = "alloc")]
use super::ParseErrorKind::BadFormat;
use super::scan;
use super::{BAD_FORMAT, INVALID, OUT_OF_RANGE, TOO_LONG, TOO_SHORT};
use super::{Fixed, InternalFixed, InternalInternal, Item, Numeric, Pad, Parsed};
Expand Down Expand Up @@ -359,7 +361,9 @@ where
Nanosecond => (9, false, Parsed::set_nanosecond),
Timestamp => (usize::MAX, false, Parsed::set_timestamp),

// for the future expansion
#[cfg(feature = "alloc")]
Padded { .. } => return Err(ParseError(BadFormat)),

Internal(ref int) => match int._dummy {},
};

Expand Down Expand Up @@ -1580,6 +1584,88 @@ mod tests {
);
}

#[test]
#[rustfmt::skip]
#[cfg(feature = "alloc")]
fn test_parse_padded() {
use crate::format::InternalInternal::*;
use crate::format::Item::{Literal, Space};
use crate::format::Numeric::*;
use crate::format::ParseErrorKind::BadFormat;

check(
"2000-01-02 03:04:05Z",
&[
nums(Year.with_padding(5))
],
Err(ParseError(BadFormat)),
);

check(
"2000-01-02 03:04:05Z",
&[
nums(Year.with_padding(5)), Literal("-"), num(Month), Literal("-"), num(Day), Space(" "),
num(Hour), Literal(":"), num(Minute), Literal(":"), num(Second),
internal_fixed(TimezoneOffsetPermissive)
],
Err(ParseError(BadFormat)),
);

check(
"2000-01-02 03:04:05Z",
&[
num(Year), Literal("-"), num(Month), Literal("-"), nums(Day.with_padding(5)), Space(" "),
num(Hour), Literal(":"), num(Minute), Literal(":"), num(Second),
internal_fixed(TimezoneOffsetPermissive)
],
Err(ParseError(BadFormat)),
);
}

#[test]
#[rustfmt::skip]
#[cfg(not(feature = "alloc"))]
fn test_parse_padded_disabled() {
use crate::format::InternalInternal::*;
use crate::format::Item::{Literal, Space};
use crate::format::Numeric::*;
use crate::format::ParseErrorKind::TooLong;

check(
"2000-01-02 03:04:05Z",
&[
nums(Year.with_padding(5))
],
Err(ParseError(TooLong)),
);

check(
"2000-01-02 03:04:05Z",
&[
nums(Year.with_padding(5)), Literal("-"), num(Month), Literal("-"), num(Day), Space(" "),
num(Hour), Literal(":"), num(Minute), Literal(":"), num(Second),
internal_fixed(TimezoneOffsetPermissive)
],
parsed!(
year: 2000, month: 1, day: 2, hour_div_12: 0, hour_mod_12: 3, minute: 4, second: 5,
offset: 0
),
);

check(
"2000-01-02 03:04:05Z",
&[
num(Year), Literal("-"), num(Month), Literal("-"), nums(Day.with_padding(5)), Space(" "),
num(Hour), Literal(":"), num(Minute), Literal(":"), num(Second),
internal_fixed(TimezoneOffsetPermissive)
],
parsed!(
year: 2000, month: 1, day: 2, hour_div_12: 0, hour_mod_12: 3, minute: 4, second: 5,
offset: 0
),
);
}

#[track_caller]
fn parses(s: &str, items: &[Item]) {
let mut parsed = Parsed::new();
Expand Down
Loading