test: Avoid F541 (f-string without any placeholders)

maflcko commented at 1:34 pm on December 4, 2024: member

An extra f string-prefix is mostly harmless, but could be confusing or hint to a mistake where a format argument was forgotten.

Try to avoid the confusion and mistakes by applying the F541 linter rule.

DrahtBot commented at 1:34 pm on December 4, 2024: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31417.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	lucasbalieiro, danielabrozzoni, tdb3

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28676 ([WIP] Cluster mempool implementation by sdaftuar)

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.

DrahtBot added the label Tests on Dec 4, 2024

lucasbalieiro approved

lucasbalieiro commented at 4:17 am on December 5, 2024: none

Hi @maflcko, ~~Tested ACK 531621d5~~

Tested ACK bd8312f

Thank you for addressing this. I understand the rationale behind the change: preventing potential confusion caused by f-strings without placeholders and mitigating mistakes where format arguments may have been inadvertently omitted.

While this is indeed a minor and harmless modification, I tested the changes by rebuilding the source and running the modified test suite. Everything functioned as expected.

0lsb_release -a
1No LSB modules are available.
2Distributor ID:	Ubuntu
3Description:	Ubuntu 22.04.5 LTS
4Release:	22.04
5Codename:	jammy
6
7python3 --version
8Python 3.10.12

Below is a screenshot of the test output for verification:

Ruff rule F541 documentation:

An f-string without placeholders can be confusing for readers, who may expect such a placeholder to be present. An f-string without any placeholders could also indicate that the author forgot to add a placeholder expression.
PEP 498:

The parts of the f-string outside of braces are literal strings.

This change aligns well with best practices and coding standards, ensuring improved readability and reducing potential errors in the future.

Thanks again for your contribution!

maflcko force-pushed on Dec 5, 2024

maflcko commented at 7:35 am on December 5, 2024: member

Tested ACK 531621d5

I don’t think this commit hash is right (531621d5 doesn’t exist). It would be bd8312fc31, but I just force pushed to add a new hunk to the diff.

maflcko force-pushed on Dec 5, 2024

DrahtBot commented at 7:38 am on December 5, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/33958434869

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot added the label CI failed on Dec 5, 2024

maflcko force-pushed on Dec 5, 2024

test: Avoid F541 (f-string without any placeholders) fae76393bd

maflcko force-pushed on Dec 5, 2024

DrahtBot removed the label CI failed on Dec 5, 2024

lucasbalieiro commented at 12:53 pm on December 5, 2024: none

Tested ACK fae7639

Hi @maflcko,

Apologies for the confusion in my earlier comment. I mistakenly referenced only the last eight characters of the commit hash instead of the first seven, which may have caused some misunderstanding.

To clarify, I have re-run all the tests, thoroughly verified the correct commit hash, and can confirm that everything works as expected:

Thank you for your understanding, and I apologize for any inconvenience caused.

danielabrozzoni approved

danielabrozzoni commented at 7:28 pm on December 5, 2024: contributor

ACK fae76393bdbf867176e65447799d6ee3d3567b18

tdb3 approved

tdb3 commented at 1:56 am on December 6, 2024: contributor

Code review ACK fae76393bdbf867176e65447799d6ee3d3567b18

The style guidelines prefer f-string over .format(), but it’s a preference and the minimal change in the PR seems reasonable when compared to something decomposed like the following:

 0diff --git a/contrib/tracing/log_raw_p2p_msgs.py b/contrib/tracing/log_raw_p2p_msgs.py
 1index 9cda7fd08d4..864847983a9 100755
 2--- a/contrib/tracing/log_raw_p2p_msgs.py
 3+++ b/contrib/tracing/log_raw_p2p_msgs.py
 4@@ -117,19 +117,13 @@ int trace_outbound_message(struct pt_regs *ctx) {
 5 
 6 
 7 def print_message(event, inbound):
 8-    print("{} {} msg '{}' from peer {} ({}, {}) with {} bytes: {}".format(
 9-
10-              "Warning: incomplete message (only {} out of {} bytes)!".format(
11-                  len(event.msg), event.msg_size) if len(event.msg) < event.msg_size else "",
12-              "inbound" if inbound else "outbound",
13-              event.msg_type.decode("utf-8"),
14-              event.peer_id,
15-              event.peer_conn_type.decode("utf-8"),
16-              event.peer_addr.decode("utf-8"),
17-              event.msg_size,
18-              bytes(event.msg[:event.msg_size]).hex(),
19-          )
20-          )
21+    warning = "Warning: incomplete message (only {len(event.msg)} out of {event.msg_size} bytes)!" if len(event.msg) < event.msg_size else ""
22+    direction = "inbound" if inbound else "outbound"
23+    msg_type = event.msg_type.decode("utf-8")
24+    conn_type = event.peer_conn_type.decode("utf-8")
25+    peer_addr = event.peer_addr.decode("utf-8")
26+    num_bytes = bytes(event.msg[:event.msg_size]).hex()
27+    print(f"{warning} {direction} msg '{msg_type}' from peer {event.peer_id} ({conn_type}, {peer_addr}) with {event.msg_size} bytes: {num_bytes}")
28 
29 
30 def main(pid):

Use f’{x}’ for string formatting in preference to ‘{}’.format(x) or ‘%s’ % x. https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines

fanquake merged this on Dec 6, 2024

fanquake closed this on Dec 6, 2024

maflcko deleted the branch on Dec 6, 2024

test: Avoid F541 (f-string without any placeholders) #31417

Code Coverage & Benchmarks

Reviews

Conflicts