util: Rename DecodeDumpTime to ParseISO8601DateTime #17266

pull elichai wants to merge 2 commits into bitcoin:master from elichai:2019-10-FormatISO8601DateTime changing 4 files +26 −16

elichai commented at 6:13 PM on October 26, 2019: contributor
As discussed in #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.
elichai renamed this:
~~Rename DecodeDumpTime to ParseISO8601DateTime~~
Wallet+util: Rename DecodeDumpTime to ParseISO8601DateTime
on Oct 26, 2019
elichai force-pushed 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:

int64_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);