Skip to content

Conversation

philolo1
Copy link
Contributor

This commit resolves #40.
Adds new file and functions parse_timestamp.
Adds tests for handling negative numbers.

@tertsdiepraam @sylvestre

Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

Lovely! I have some minor comments, but it's looking pretty good!

@philolo1
Copy link
Contributor Author

@tertsdiepraam adjusted the code.

use nom::sequence::tuple;
use nom::{self, IResult};

pub(crate) fn parse_timestamp(s: &str) -> Option<i64> {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we return:
Result<i64, ParseTimestampError>

like we return ParseDateTimeError elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

let res: IResult<&str, (char, &str)> = all_consuming(preceded(
char('@'),
tuple((
// Note: gnu date allows multiple + and - and only considers the last one
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add that we are doing the same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -0,0 +1,67 @@
use nom::branch::alt;
Copy link
Contributor

Choose a reason for hiding this comment

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

please add the license header

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

src/lib.rs Outdated
return Ok(dt);
}
if let Some(timestamp) = parse_timestamp(s.as_ref()) {
let timestamp_date = NaiveDateTime::from_timestamp_opt(timestamp, 0).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens in case of error ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

This commit resolves uutils#40.
Adds new file and functions parse_timestamp.
Adds tests for handling negative numbers.
@philolo1
Copy link
Contributor Author

philolo1 commented Sep 4, 2023

@sylvestre updated, let me know if you like to resolve the comments, or i should. I am not sure how you handle it.

@philolo1
Copy link
Contributor Author

@tertsdiepraam @sylvestre just checking on the status. I think someone needs to review it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Negative numbers for @ are not being recognized
3 participants