Support time duration more than 23

[nix010]

Fix #63 . support for pydantic/pydantic#8673

[codecov]

Codecov Report

Attention: Patch coverage is 92.68293% with 3 lines in your changes missing coverage. Please review.

Files	Patch %	Lines
src/duration.rs	92.68%	3 Missing ⚠️

📢 Thoughts on this report? Let us know!

[davidhewitt]

Thanks, overall this looks fine though I have some simplifications which I think we should consider.

[davidhewitt]

This can be a constant.

Suggested change

	let hour_numeric_limit = 24 * (1e9 as i64);
	const HOUR_NUMERIC_LIMIT: i64 = 24 * 10i64.pow(8);

[davidhewitt]

Suggested change

	let minute_and_second_part = bytes[byte_len - 5..].to_vec();
	let mut t = Self::parse_minutes_seconds(&minute_and_second_part[..], 0)?;
	let mut t = Self::parse_minutes_seconds(&bytes[byte_len-5..], 0)?;

[davidhewitt]

Is offset ever anything other than 0 here? I think it makes more sense to just take bytes as the single argument (and let the caller adjust the slice passed in if necessary).

[davidhewitt]

Is parse_time only called here? If so, perhaps it makes sense to just merge parse_time into parse_oversize_hour_time_format? (And call it parse_hours_minutes_seconds?)

i.e. I wonder if this can all be simplified and we just do the check currently on line 312 and stop bothering to check bytes.get(offset + 2) up front.

[nix010]

I just rework on this and think I can just bring the logic into the parse_time func itself. let's me know what you think.

[davidhewitt]

Overall logic looks great!

I have a few style suggestions and simplifications. Also there's some lint job failures. Some are related to this PR and some are caused by newer Rust version coming through. Can you fix those please?

After that's all addressed, I think this should be good to merge, thanks 👍

[davidhewitt]

Suggestion to make this terser:

Suggested change

	fn is_duration_date_format(bytes: &[u8]) -> bool {
	for byte in bytes {
	if *byte == b'd' || *byte == b'D' {
	return true;
	}
	}
	false
	}
	fn is_duration_date_format(bytes: &[u8]) -> bool {
	bytes.iter().any(|&byte| byte == b'd' || byte == b'D')
	}

[davidhewitt]

Following the adjustment above, we can just iterate the hour part:

Suggested change

	for idx in offset..first_colon_index {
	let h = bytes.get(idx).ok_or(ParseError::InvalidCharHour)? - b'0';
	if h > 9 {
	return Err(ParseError::InvalidCharHour);
	}
	for byte in hour_part {
	let h = *byte - b'0';
	if h > 9 {
	return Err(ParseError::InvalidCharHour);
	}

[davidhewitt]

We shouldn't unwrap here. Probably we should return either TooShort or InvalidCharHour. Let's also simplify the rest of the code by doing some slice manipulation here:

Suggested change

	let first_colon_index = bytes.iter().position(|&x| x == b':').unwrap();
	let mut chunks = bytes
	.get(offset..)
	.ok_or(ParseError::TooShort)?
	.splitn(2, |&byte| byte == b':');
	// can just use `.split_once()` in future maybe, if that stabilises
	let (hour_part, remaining) = match (chunks.next(), chunks.next(), chunks.next()) {
	(_, _, Some(_)) | (None, _, _) => unreachable!("should always be 1 or 2 chunks"),
	(Some(hour_part), None, _) => return Err(ParseErr::InvalidCharHour),
	(Some(hour_part), Some(remaining), None) => (hour_part, remaining),
	};

[davidhewitt]

This check can be moved up before splitting on the colon.

[davidhewitt]

Given we know the size of what we're trying to parse here, we can make things a lot more efficient by avoiding the dynamic size & allocation of the vector:

Suggested change

	let mut temp_vec: Vec<u8> = vec![b'0', b'0'];
	temp_vec.extend_from_slice(&bytes[first_colon_index..]);
	let new_bytes = &temp_vec[..];
	// remaining minutes and seconds part
	if remaining.len() > 5 {
	return Err(ParseError::ExtraCharacters());
	}
	let mut new_bytes = *b"00:00:00";
	let new_bytes = &mut new_bytes[... 3 + remaining.len()];
	new_bytes[3..].copy_from_slice(remaining);

[nix010]

@davidhewitt actually there's also a case with fraction of second (microsecond) so that why I just want to set the hour part as 00.

[davidhewitt]

Understood. Does it work if we generalize my suggestion to b"00:00:00.000000" instead?

[nix010]

I don't know but the fraction part will always have to have 6 chars ? if so then yes.

[davidhewitt]

Because we slice it with &mut new_bytes[... 3 + remaining.len()] then we won't actually use any of the zeros beyond what's actually in the remaining data.

And I think our limit is microseconds (i.e. max 6 chars). So this should be ok, I think.

[nix010]

@davidhewitt ok. I made the change but had a trouble with a test test_duration_parse_truncate_seconds. Can you help me with that. It's fraction issue.

[davidhewitt]

Sure thing, #64 (comment)

[davidhewitt]

Let's add a test with microseconds, perhaps, given the new discussion above.

[nix010]

@davidhewitt btw, the lint job failed with the README file so how should I resolve it ?

error: needless `fn main` in doctest
  --> src/../README.md:43:1

[davidhewitt]

Just remove the fn main() { and de-indent the contents from the offending snippets.

e.g.

fn main() {
    foo
}

becomes

foo

[davidhewitt]

I think we can just follow what happens inside parse_time;

Suggested change

	return Err(ParseError::SecondFractionTooLong);
	match config.microseconds_precision_overflow_behavior {
	crate::MicrosecondsPrecisionOverflowBehavior::Truncate => remaining = &remaining[..new_bytes.len() - 3],
	crate::MicrosecondsPrecisionOverflowBehavior::Error => return Err(ParseError::SecondFractionTooLong),
	}

[nix010]

@davidhewitt Thank you! all work now. I'm learning rust by doing these PRs and gain a lot of your suggesions. Cheers.

[davidhewitt]

No problem, I think this is overall looking straightforward & efficient now! Been a pleasure to collaborate on this one and glad the feedback has been useful.

I think I've just spotted a final case, and then once we've decided what to do about that then let's merge 🚀

[davidhewitt]

Just as a final thought here, I think it's possible for us to overflow if hour_part is too many bytes.

I think in a previous version you had a check on the maximum length of hour_part, probably we just dropped that during rounds of simplification. Maybe we should reintroduce that?

[nix010]

nice catch. I just added that in again.

[davidhewitt]

Looks super! Many thanks for all the iteration put in here 🎉