-
Notifications
You must be signed in to change notification settings - Fork 37.7k
test: consolidate to f-strings (part 1) #22229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Concept ACK |
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. |
Concept ACK |
Concept ACK and good idea. Are you using a tool (flynt?) to do this? I guess it requires a good amount of review since there are some cases where there are discrepancies between string formats. I’ll trying running flynt and diffing it against this PR to see that in the next day or so, and give this some more review. I may also trying diffing test logs before/after these changes. Will get back after that. Any thoughts on squashing commits? |
Concept ACK good GOHIO (get our house in order) PR |
Concept ACK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK, went through line by line, wasn't as much/bad as I expected
Also ran tests and scrolled through (skimming them) and didn't see anything weird that would indicate a malformed string.
Concept ACK |
705d503
to
68faa87
Compare
Rebased, addressed comment, and squashed these down a bit. |
reACK 68faa87 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
crACK 68faa87
@@ -114,7 +114,7 @@ def run_test(self): | |||
assert_equal(self.nodes[1].getbalance(), Decimal("0.1")) | |||
|
|||
# Check chainTip response | |||
json_obj = self.test_rest_request("/getutxos/{}-{}".format(*spending)) | |||
json_obj = self.test_rest_request(f"/getutxos/{spending[0]}-{spending[1]}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While f-strings are preferred, I think here it is subjective which version is better. Since our dev notes don't require a specific style, I think it is fine to not use f-strings everywhere.
Also, I'd slightly prefer if the conflicts (#22229 (comment)) are first dealt with before part 2 (etc) are opened. |
Rather than using 3 different ways to build/format strings (sometimes all in the same test, i.e
feature_config_args.py
), consolidate to using f-strings (3.6+), which are generally more concise / readable, as well as more performant than existing methods.This deals with the
feature_*.py
,interface_*.py
andmining_*.py
tests.See also: PEP 498