refactor: use f-strings across codebase #768
Conversation
|
1000+ lines of changes 😰 I looked at maybe 100+ of them, all looks ok 😄 |
|
A few comments to address, let's get this merged! |
I actually tried implementing your comments (not just the lines you mentioned but in regards to the pattern) and messed everything up because I got confused with the types. That's why I focused on the "Type Hints" PR first and wanted to fix this one afterwards. |
|
OK fair |
|
@theguy147 I will stop developing on this branch (after 5f641bb) so you can now adjust the conflicts. Some more commits might land, but only affect Cheers |
theguy147
force-pushed
the
gdb_8_py36_code_refactor
branch
2 times, most recently
from
076d565
to
ef19244
Compare
theguy147
force-pushed
the
gdb_8_py36_code_refactor
branch
2 times, most recently
from
4238d69
to
7b71465
Compare
… gdb_8_py36_code_refactor
| # backward compat for gdb (gdb < 7.10) | ||
| if not hasattr(gdb, "FrameDecorator"): | ||
| gdb.execute("backtrace {:d}".format(nb_backtrace)) | ||
| gdb.execute(f"backtrace {nb_backtrace:d}") | ||
| return |
There was a problem hiding this comment.
It has nothing to do with your PR but since we now baseline gdb to be 8, can you remove this block?
| # backward compat for gdb (gdb < 7.10) | |
| if not hasattr(gdb, "FrameDecorator"): | |
| gdb.execute("backtrace {:d}".format(nb_backtrace)) | |
| gdb.execute(f"backtrace {nb_backtrace:d}") | |
| return |
There was a problem hiding this comment.
Added to project https://github.com/hugsy/gef/projects/11#card-75928305
There was a problem hiding this comment.
So should I still commit the suggested fix or now that it was added to the project to track, we do that in a future code change?
| memalign = gef.arch.ptrsize | ||
|
|
||
| offset = idx * memalign | ||
| current_address = align_address(addr + offset) | ||
| addrs = dereference_from(current_address) | ||
| l = "" | ||
| addr_l = format_address(int(addrs[0], 16)) | ||
| l += "{:s}{:s}{:+#07x}: {:{ma}s}".format(Color.colorify(addr_l, base_address_color), | ||
| l += "{}{}{:+#07x}: {:{ma}s}".format(Color.colorify(addr_l, base_address_color), |
There was a problem hiding this comment.
Any reason you didn't f-string this one?
There was a problem hiding this comment.
Good eye, also the args on the next line aren't lined up.
There was a problem hiding this comment.
There are a few format-strings that I didn't convert to f-strings yet. When I didn't convert them it was mostly because I was concerned with worsening the readability. In this case the part I wasn't sure about is {:{ma}s}. This line would turn into:
l += f"{Color.colorify(addr_l, base_address_color)}{VERTICAL_LINE}{base_offset+offset:+#07x}: {sep.join(addrs[1:]):{memalign*2+2}s}"I can include this and similar strings as well if you think that it improves the code quality overall (at least for the aspect of consistency)
There was a problem hiding this comment.
Yeah fair. We shouldn't be married to f strings if it makes things harder to read (but please fix the alignment of the args)
refactor: use f-strings across codebase
Description/Motivation/Screenshots
This PR replaces all
.format(...)and%strings with f-strings.How Has This Been Tested?
make testtest_cmd_elf_infoalready failed on this branch before this PRChecklist
gdb_8_py36_code_refactorbranch, notmaster.