util: Rename DecodeDumpTime to ParseISO8601DateTime #17266
Conversation
ce4b0cd to
6fe0863
Compare
|
(Fixed the whitespaces already. not sure what's up with travis) |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
6fe0863 to
e7b02b5
Compare
|
ACK e7b02b5 |
| if (ptime.is_not_a_date_time() || epoch > ptime) | ||
| return 0; | ||
| return (ptime - epoch).total_seconds(); | ||
| } |
There was a problem hiding this comment.
Nit: Missing newline at end of file :)
| @@ -145,9 +145,17 @@ BOOST_AUTO_TEST_CASE(util_Join) | |||
| BOOST_CHECK_EQUAL(Join<std::string>({"foo", "bar"}, ", ", op_upper), "FOO, BAR"); | |||
| } | |||
|
|
|||
| BOOST_AUTO_TEST_CASE(util_FormatISO8601DateTime) | |||
| BOOST_AUTO_TEST_CASE(util_FormatParseISO8601DateTime) | |||
There was a problem hiding this comment.
What is FormatParseISO8601DateTime? I suggest add a separate test case for ParseISO8601DateTime.
There was a problem hiding this comment.
Well, if I'll do so at which one of them should I add the round trip test? :)
There was a problem hiding this comment.
I think testing both in one test case is fine in this case. It's not like it's a huge function that needs to be broken up.
There was a problem hiding this comment.
Yeah it's fine, just that we name cases like "util_{function}".
|
|
||
| BOOST_CHECK_EQUAL(ParseISO8601DateTime("1970-01-01T00:00:00Z"), 0); | ||
| BOOST_CHECK_EQUAL(ParseISO8601DateTime("1960-01-01T00:00:00Z"), 0); | ||
| BOOST_CHECK_EQUAL(ParseISO8601DateTime("2011-09-30T23:36:17Z"), 1317425777); |
There was a problem hiding this comment.
Consider adding some malformed inputs too to make sure we're not only testing the happy paths :)
There was a problem hiding this comment.
Well apparently boost overflows the time (i.e. 70 minutes). so the malformed ones are a problem.
I might try to fix that in a future PR together with more test cases
|
I think this is good to go. Let's not make a PHD thesis out of a refactoring pull. ACK e7b02b5 Show signature and timestampSignature: Timestamp of file with hash |
e7b02b5 Add roundtrip and more tests to ParseISO8601DateTime and FormatISO8601DateTime (Elichai Turkel) 9e2c623 Rename DecodeDumpTime to ParseISO8601DateTime and move to time.cpp (Elichai Turkel) Pull request description: As discussed in bitcoin#17245. 1. Renamed the function. 2. Moved it from `rpcdump.cpp` to `time.cpp`. 3. Added a check if the time is less then epoch return 0 to prevent an overflow. 4. Added more edge cases tests and a roundtrip test. ACKs for top commit: laanwj: ACK e7b02b5 MarcoFalke: ACK e7b02b5 promag: Code review ACK e7b02b5. Moved code is correct, left a comment regarding the test change. Tree-SHA512: 703c21e09b2aabc992235149e67acba63d9d77a593ec8f6d2fec3eb63a7e5c406d56cbce6c6513ab32fba43367d073d2345f3b589843e3c5fe4f55ea3e00bf29
Summary: * Add roundtrip and more tests to ParseISO8601DateTime and FormatISO8601DateTime This is a backport of Core [[bitcoin/bitcoin#17266 | PR17266]] Test Plan: ninja check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D5447
Summary: * Add roundtrip and more tests to ParseISO8601DateTime and FormatISO8601DateTime This is a backport of Core [[bitcoin/bitcoin#17266 | PR17266]] Test Plan: ninja check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D5447
As discussed in #17245.
rpcdump.cpptotime.cpp.