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

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

Contributors
adaminsky theStack MarcoFalke niVelion DrahtBot

Labels
Tests Needs rebase