Skip to content

Conversation

fanquake
Copy link
Member

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 and mining_*.py tests.

See also: PEP 498

@fanquake fanquake added the Tests label Jun 12, 2021
@practicalswift
Copy link
Contributor

Concept ACK

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 12, 2021

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #22567 (test: Implicitly sync after generate* to preempt races and intermittent test failures by MarcoFalke)
  • #20362 (test: Implicitly sync after generate* to preempt races and intermittent test failures by MarcoFalke)

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.

@theStack
Copy link
Contributor

Concept ACK

@mjdietzx
Copy link
Contributor

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?

@josibake
Copy link
Member

Concept ACK

good GOHIO (get our house in order) PR

@Zero-1729
Copy link
Contributor

Concept ACK

Copy link
Contributor

@mjdietzx mjdietzx 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, 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.

@jamesob
Copy link
Contributor

jamesob commented Aug 17, 2021

Concept ACK

@fanquake fanquake force-pushed the consolidate_to_f_strings branch from 705d503 to 68faa87 Compare August 18, 2021 04:39
@fanquake
Copy link
Member Author

Rebased, addressed comment, and squashed these down a bit.

@mjdietzx
Copy link
Contributor

reACK 68faa87

Copy link
Contributor

@Zero-1729 Zero-1729 left a 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]}")
Copy link
Member

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.

@maflcko
Copy link
Member

maflcko commented Aug 18, 2021

Also, I'd slightly prefer if the conflicts (#22229 (comment)) are first dealt with before part 2 (etc) are opened.

@fanquake fanquake deleted the consolidate_to_f_strings branch August 18, 2021 23:44
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 20, 2021
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants