test: Add consistent formatting for messages.py repr methods #19889
Conversation
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".
|
Concept ACK -- consistent formatting and more clarity w.r.t. numbers base (hex vs decimal) are definitely desirable |
| return r | ||
|
|
||
| def __repr__(self): | ||
| return "CAddress(nServices=%i ip=%s port=%i)" % (self.nServices, |
There was a problem hiding this comment.
Any reason to not allow this constructor?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
🐙 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". |
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
|
Concept ACK, looks like a great improvement. Happy to test and review changes after rebase. |
|
Closing for now. This can be reopened any time or a new pull can be created (either by this author or another author). |
6 participants
This PR tries to make the
__repr__methods for messages and primitives more consistent intest/functional/test_framework/messages.pyby makingreprreturn 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.