util: Rename DecodeDumpTime to ParseISO8601DateTime

elichai commented at 6:13 pm on October 26, 2019: contributor

As discussed in #17245.

Renamed the function.
Moved it from rpcdump.cpp to time.cpp.
Added a check if the time is less then epoch return 0 to prevent an overflow.
Added more edge cases tests and a roundtrip test.

elichai renamed this:
~~Rename DecodeDumpTime to ParseISO8601DateTime~~
Wallet+util: Rename DecodeDumpTime to ParseISO8601DateTime
on Oct 26, 2019

elichai force-pushed on Oct 26, 2019

MarcoFalke added the label Refactoring on Oct 26, 2019

MarcoFalke renamed this:
~~Wallet+util: Rename DecodeDumpTime to ParseISO8601DateTime~~
util: Rename DecodeDumpTime to ParseISO8601DateTime
on Oct 26, 2019

elichai commented at 6:59 pm on October 26, 2019: contributor

(Fixed the whitespaces already. not sure what’s up with travis)

DrahtBot commented at 7:06 pm on October 26, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16037 (rpc: Enable wallet import on pruned nodes by promag)

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.

Rename DecodeDumpTime to ParseISO8601DateTime and move to time.cpp 9e2c623be5

Add roundtrip and more tests to ParseISO8601DateTime and FormatISO8601DateTime e7b02b54cc

in src/util/time.h:51 in 6fe08631a8 outdated

47@@ -48,5 +48,6 @@ T GetTime();
48  */
49 std::string FormatISO8601DateTime(int64_t nTime);
50 std::string FormatISO8601Date(int64_t nTime);
51+int64_t ParseISO8601DateTime(const std::string &str);

MarcoFalke commented at 9:12 pm on October 26, 2019:

0int64_t ParseISO8601DateTime(const std::string& str);

You may remove trailing whitespace with clang-format by running the https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-format-diffpy script

elichai commented at 10:01 pm on October 26, 2019:

Thanks :)

elichai force-pushed on Oct 26, 2019

laanwj commented at 9:47 am on October 27, 2019: member

ACK e7b02b54ccfb6b2e119a67799220f8d8d8b5cccd

in src/util/time.cpp:127 in e7b02b54cc

122+    boost::posix_time::ptime ptime(boost::date_time::not_a_date_time);
123+    iss >> ptime;
124+    if (ptime.is_not_a_date_time() || epoch > ptime)
125+        return 0;
126+    return (ptime - epoch).total_seconds();
127+}

practicalswift commented at 8:57 pm on October 27, 2019:

Nit: Missing newline at end of file :)

practicalswift approved

practicalswift commented at 8:58 pm on October 27, 2019: contributor

ACK modulo nit

in src/test/util_tests.cpp:148 in e7b02b54cc

144@@ -145,9 +145,17 @@ BOOST_AUTO_TEST_CASE(util_Join)
145     BOOST_CHECK_EQUAL(Join<std::string>({"foo", "bar"}, ", ", op_upper), "FOO, BAR");
146 }
147 
148-BOOST_AUTO_TEST_CASE(util_FormatISO8601DateTime)
149+BOOST_AUTO_TEST_CASE(util_FormatParseISO8601DateTime)

promag commented at 8:29 am on October 28, 2019:

What is FormatParseISO8601DateTime? I suggest add a separate test case for ParseISO8601DateTime.

elichai commented at 8:54 am on October 28, 2019:

Well, if I’ll do so at which one of them should I add the round trip test? :)

promag commented at 9:02 am on October 28, 2019:

To the new case IMO.

laanwj commented at 10:47 am on October 28, 2019:

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.

promag commented at 10:50 am on October 28, 2019:

Yeah it’s fine, just that we name cases like “util_{function}”.

promag commented at 8:30 am on October 28, 2019: member

Code review ACK e7b02b54ccfb6b2e119a67799220f8d8d8b5cccd. Moved code is correct, left a comment regarding the test change.

in src/test/util_tests.cpp:155 in e7b02b54cc

151     BOOST_CHECK_EQUAL(FormatISO8601DateTime(1317425777), "2011-09-30T23:36:17Z");
152+    BOOST_CHECK_EQUAL(FormatISO8601DateTime(0), "1970-01-01T00:00:00Z");
153+
154+    BOOST_CHECK_EQUAL(ParseISO8601DateTime("1970-01-01T00:00:00Z"), 0);
155+    BOOST_CHECK_EQUAL(ParseISO8601DateTime("1960-01-01T00:00:00Z"), 0);
156+    BOOST_CHECK_EQUAL(ParseISO8601DateTime("2011-09-30T23:36:17Z"), 1317425777);

practicalswift commented at 11:50 am on October 28, 2019:

Consider adding some malformed inputs too to make sure we’re not only testing the happy paths :)

elichai commented at 2:16 pm on October 28, 2019:

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

MarcoFalke commented at 2:30 pm on October 28, 2019: member

I think this is good to go. Let’s not make a PHD thesis out of a refactoring pull.

ACK e7b02b54ccfb6b2e119a67799220f8d8d8b5cccd

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK e7b02b54ccfb6b2e119a67799220f8d8d8b5cccd
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjLfgv+N9UU0SzxAKf0FRYvyT/bhaXaiZZ6WgODU8MZffV9imON/+O47MsTOF12
 8If2MPrpgSn9KUlqqnfRrYEPkrvC6GG//w63vmZ6JpDgwIQptgZPvddxJF3FPOvay
 96sptCfj/ntE+EYnb2Jb3Zw+1FOuSt5le36KbcNlEzSC3eDK71THEZ5/KqwvmCLFD
10CSdVzTCfh3wq+FZdChQUnpzb5E8BKTFjw3oKp9Sph9GJ6DCBZlMEb95LjJnZEx5i
11u3BDMq8TtC2xNDDwm6WllsFhMqWufXwhwUEy+ARK08xd9/HpovrKHax27aSEf/qc
12UG9bf4ITedUh6zKgF9W4C15gnWa9QL/gtyq+bMIC4Rfzl8ouqyd50h/JZ0/9s6x1
13xC60XeHgEWe+o1TRTS6FqN40Pn6+ijSo/Mvf1+9wZfBdI6rY4424qX/SeVbir8B1
14uZ6MLnXRT+Ssn5f3xEJEjoV9sCpkrCEwGyYRFEpGy4ANyOh57TDihfA1zM8Su1xs
15ThCJLOPH
16=BFqQ
17-----END PGP SIGNATURE-----

Timestamp of file with hash 7d5daa2b5604084f8abd3f86e9acbccc87bc5f25ecf25d02fa93428c29c4752d -

MarcoFalke referenced this in commit cfec3e01b4 on Oct 28, 2019

MarcoFalke merged this on Oct 28, 2019

MarcoFalke closed this on Oct 28, 2019

elichai deleted the branch on Oct 28, 2019

deadalnix referenced this in commit c0f5ddc4a2 on Mar 10, 2020

ftrader referenced this in commit e723b38434 on May 19, 2020

kittywhiskers referenced this in commit e2da3fff73 on Feb 27, 2022

kittywhiskers referenced this in commit a67bd2aa36 on Feb 28, 2022

kittywhiskers referenced this in commit 5e504e88bf on Feb 28, 2022

kittywhiskers referenced this in commit 07d7c63cdf on Feb 28, 2022

kittywhiskers referenced this in commit df72207552 on Mar 13, 2022

kittywhiskers referenced this in commit bbbba97fb6 on Mar 24, 2022

gades referenced this in commit d19e3858f2 on May 19, 2022

DrahtBot locked this on Aug 16, 2022

util: Rename DecodeDumpTime to ParseISO8601DateTime #17266

Conflicts