test: Add consistent formatting for messages.py repr methods #19889

pull adaminsky wants to merge 1 commits into bitcoin:master from adaminsky:printstr_bug changing 1 files +85 −72
  1. adaminsky commented at 11:40 pm on September 5, 2020: contributor

    This PR tries to make the __repr__ methods for messages and primitives more consistent in test/functional/test_framework/messages.py by making repr return a string with a valid constructor call to the message/primitive if that is possible, or returning information in angle brackets to show the internal state of the message/primitive.

    Also, all hashes and other hex data is prefixed with “0x” and the correct minimum length of the hex string is set with the format string. I think the leading “0x” can be useful since then it is obvious what is an integer vs. hex. These functions are only used for logging, so this is meant to make it clearer how to reconstruct an object if necessary and what the size of the printed hex types are.

  2. Clean up messages.py repr methods
    Some of the repr methods return actual executable code which could be
    used to recreate the exact object and some of them just represent the
    interal state of the object being printed. For the objects which do not
    have a repr string that is a valid constructor call, I use angle
    brackets to show that it is the internal state of the object being
    printed and not a valid way to construct the object. This commit also
    changes the `%` style formatting to use `.format` Python formatting and
    prefixes all printed hex with "0x".
    bf8c09d427
  3. fanquake added the label Tests on Sep 5, 2020
  4. theStack commented at 9:53 am on September 12, 2020: member
    Concept ACK – consistent formatting and more clarity w.r.t. numbers base (hex vs decimal) are definitely desirable
  5. fanquake requested review from MarcoFalke on Sep 21, 2020
  6. in test/functional/test_framework/messages.py:234 in bf8c09d427
    230@@ -231,8 +231,8 @@ def serialize(self, *, with_time=True):
    231         return r
    232 
    233     def __repr__(self):
    234-        return "CAddress(nServices=%i ip=%s port=%i)" % (self.nServices,
    


    MarcoFalke commented at 8:50 pm on September 22, 2020:
    Any reason to not allow this constructor?

    adaminsky commented at 10:40 pm on September 22, 2020:
    It seems like the common pattern to create CAddress objects is to call CAddress() and then manually change time, nServices, ip, and port from their default value, so I don’t see any reason why not to add these as optional arguments to the constructor. I think a couple other primitives could have their constructor take optional arguments too.

    niVelion commented at 11:45 am on January 17, 2022:

    I agree it’s a common pattern to, to give another example, for a CTransaction with nVersion 2 to be created by calling CTransaction() and then manually changing nVersion.

    However, I think it’s out of scope for this PR to change how objects are instantiated and configured.

  7. DrahtBot commented at 11:12 pm on September 28, 2020: 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:

    • #19954 (Complete the BIP155 implementation and upgrade to TORv3 by vasild)

    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.

  8. DrahtBot added the label Needs rebase on Oct 11, 2020
  9. DrahtBot commented at 2:23 am on October 11, 2020: member

    🐙 This pull request conflicts with the target branch and needs rebase.

    Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

  10. DrahtBot commented at 11:21 am on December 15, 2021: member
    • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
    • Is it no longer relevant? ➡️ Please close.
    • Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.
  11. niVelion commented at 11:52 am on January 17, 2022: none
    Concept ACK, looks like a great improvement. Happy to test and review changes after rebase.
  12. MarcoFalke commented at 11:47 am on February 22, 2022: member
    Closing for now. This can be reopened any time or a new pull can be created (either by this author or another author).
  13. MarcoFalke closed this on Feb 22, 2022

  14. DrahtBot locked this on Feb 22, 2023

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-08 16:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me