Skip to content

util: Rename DecodeDumpTime to ParseISO8601DateTime#17266

Merged
maflcko merged 2 commits intobitcoin:masterfrom
elichai:2019-10-FormatISO8601DateTime
Oct 28, 2019
Merged

util: Rename DecodeDumpTime to ParseISO8601DateTime#17266
maflcko merged 2 commits intobitcoin:masterfrom
elichai:2019-10-FormatISO8601DateTime

Conversation

@elichai
Copy link
Contributor

@elichai elichai commented Oct 26, 2019 •
edited
Loading

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 elichai mentioned this pull request Oct 26, 2019
Closed
@elichai elichai changed the title Rename DecodeDumpTime to ParseISO8601DateTime Wallet+util: Rename DecodeDumpTime to ParseISO8601DateTime Oct 26, 2019
@elichai elichai force-pushed the 2019-10-FormatISO8601DateTime branch 2 times, most recently from ce4b0cd to 6fe0863 Compare October 26, 2019 18:18
@maflcko maflcko changed the title Wallet+util: Rename DecodeDumpTime to ParseISO8601DateTime util: Rename DecodeDumpTime to ParseISO8601DateTime Oct 26, 2019
@elichai
Copy link
Contributor Author

elichai commented Oct 26, 2019

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

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 26, 2019

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.

maflcko
maflcko reviewed Oct 26, 2019
View reviewed changes
@elichai elichai force-pushed the 2019-10-FormatISO8601DateTime branch from 6fe0863 to e7b02b5 Compare October 26, 2019 22:01
@laanwj
Copy link
Member

laanwj commented Oct 27, 2019

ACK e7b02b5

practicalswift
practicalswift approved these changes Oct 27, 2019
View reviewed changes
Copy link
Contributor

@practicalswift practicalswift left a comment

Choose a reason for hiding this comment

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

ACK modulo nit

src/util/time.cpp
if (ptime.is_not_a_date_time() || epoch > ptime)
return 0;
return (ptime - epoch).total_seconds();
} No newline at end of file
Copy link
Contributor

@practicalswift practicalswift Oct 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: Missing newline at end of file :)

promag
promag reviewed Oct 28, 2019
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

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

src/test/util_tests.cpp
}

BOOST_AUTO_TEST_CASE(util_FormatISO8601DateTime)
BOOST_AUTO_TEST_CASE(util_FormatParseISO8601DateTime)
Copy link
Contributor

@promag promag Oct 28, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@elichai elichai Oct 28, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@promag promag Oct 28, 2019

Choose a reason for hiding this comment

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

To the new case IMO.

Copy link
Member

@laanwj laanwj Oct 28, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@promag promag Oct 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah it's fine, just that we name cases like "util_{function}".

practicalswift
practicalswift reviewed Oct 28, 2019
View reviewed changes
src/test/util_tests.cpp

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);
Copy link
Contributor

@practicalswift practicalswift Oct 28, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@elichai elichai Oct 28, 2019

Choose a reason for hiding this comment

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

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

@maflcko
Copy link
Member

maflcko commented Oct 28, 2019

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

ACK e7b02b5

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK e7b02b54ccfb6b2e119a67799220f8d8d8b5cccd
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjLfgv+N9UU0SzxAKf0FRYvyT/bhaXaiZZ6WgODU8MZffV9imON/+O47MsTOF12
If2MPrpgSn9KUlqqnfRrYEPkrvC6GG//w63vmZ6JpDgwIQptgZPvddxJF3FPOvay
6sptCfj/ntE+EYnb2Jb3Zw+1FOuSt5le36KbcNlEzSC3eDK71THEZ5/KqwvmCLFD
CSdVzTCfh3wq+FZdChQUnpzb5E8BKTFjw3oKp9Sph9GJ6DCBZlMEb95LjJnZEx5i
u3BDMq8TtC2xNDDwm6WllsFhMqWufXwhwUEy+ARK08xd9/HpovrKHax27aSEf/qc
UG9bf4ITedUh6zKgF9W4C15gnWa9QL/gtyq+bMIC4Rfzl8ouqyd50h/JZ0/9s6x1
xC60XeHgEWe+o1TRTS6FqN40Pn6+ijSo/Mvf1+9wZfBdI6rY4424qX/SeVbir8B1
uZ6MLnXRT+Ssn5f3xEJEjoV9sCpkrCEwGyYRFEpGy4ANyOh57TDihfA1zM8Su1xs
ThCJLOPH
=BFqQ
-----END PGP SIGNATURE-----

Timestamp of file with hash 7d5daa2b5604084f8abd3f86e9acbccc87bc5f25ecf25d02fa93428c29c4752d -

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Oct 28, 2019
Merge bitcoin#17266: util: Rename DecodeDumpTime to ParseISO8601DateTime
cfec3e0 
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
@maflcko maflcko merged commit e7b02b5 into bitcoin:master Oct 28, 2019
@elichai elichai deleted the 2019-10-FormatISO8601DateTime branch October 28, 2019 14:51
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Mar 10, 2020
@elichai @deadalnix
Rename DecodeDumpTime to ParseISO8601DateTime and move to time.cpp
c0f5ddc 
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
@MarkLTZ MarkLTZ mentioned this pull request Apr 4, 2020
Closed
ftrader pushed a commit to bitcoin-cash-node/bitcoin-cash-node that referenced this pull request May 19, 2020
@elichai @ftrader
Rename DecodeDumpTime to ParseISO8601DateTime and move to time.cpp
e723b38 
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
kwvg added a commit to kwvg/dash that referenced this pull request Mar 24, 2022
@kwvg
merge bitcoin#17266: Rename DecodeDumpTime to ParseISO8601DateTime
bbbba97
gades pushed a commit to cosanta/cosanta-core that referenced this pull request May 19, 2022
@kwvg @gades
merge bitcoin#17266: Rename DecodeDumpTime to ParseISO8601DateTime
d19e385
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@laanwj laanwj laanwj left review comments

+3 more reviewers

@promag promag promag left review comments

@maflcko maflcko maflcko left review comments

@practicalswift practicalswift practicalswift approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@elichai @DrahtBot @laanwj @maflcko @promag @practicalswift