Handle %s strings with length and alignment #194
Conversation
%s format strings allow for padding and alignment, but their behaviour is very different from fstrings. %20s will pad a string to 20 characters, and right align %-20s will pad a string to 20 characters, and left align This behaviour is carried over from the C *printf() functions. This patch adds the ability to properly convert these to fstrings, using the correct alignment markers. The feature is gated behind aggressive mode for now. The earlier code did already convert %20s, but changed the alignment (the resulting fstring would be left aligned instead of right), and did not understand %-20s at all.
| def test_string_specific_len(state: State): | ||
| s_in = """'%5s' % CLASS_NAMES[labels[j]]""" | ||
| s_expected = """f'{CLASS_NAMES[labels[j]]:>5}'""" | ||
|
|
||
| state.aggressive = True | ||
| s_out, count = code_editor.fstringify_code_by_line(s_in, state) | ||
| assert s_out == s_expected | ||
|
|
||
|
|
||
| def test_string_specific_len_right_aligned(state: State): | ||
| s_in = """'%5s' % CLASS_NAMES[labels[j]]""" | ||
| s_expected = """f'{CLASS_NAMES[labels[j]]:>5}'""" | ||
|
|
||
| state.aggressive = True | ||
| s_out, count = code_editor.fstringify_code_by_line(s_in, state) | ||
| assert s_out == s_expected |
There was a problem hiding this comment.
These two tests are exactly the same? lets delete one.
There was a problem hiding this comment.
Agreed, test_string_specific_len can go?
| # In order to not have to figure out what sort of number we are | ||
| # dealing with, just look at the leading - sign (if there is one) | ||
| # and remove it for the conversion | ||
| if aggressive and fmt_prefix.startswith("-"): |
There was a problem hiding this comment.
| if aggressive and fmt_prefix.startswith("-"): | |
| if fmt_prefix.startswith("-"): |
It seems that we have handled all possible cases within this branch (fmt_spec == "s" and fmt_prefix), so there is no room for error anymore; therefore we don't need aggressive flag? aggressive means "risky", some changes that are not guaranteed to be correct.
Is there any known/suspected case where outputs might differ for original and new f-string? to me it sounds exhaustive what you've suggested.
There was a problem hiding this comment.
The reason I "locked" this behind the aggressive flag is that it changes the current behaviour. Arguably the current behaviour is wrong (a right aligned percentage string would become a left aligned fstring), but still.
If you feel that's not critical I'm fine with dropping the flag requirement.
There was a problem hiding this comment.
In my logic aggressive is not for compatibility with old version, people can freeze flynt version for this. Its to use some logic that might change how strings come out of formatting. Seems all cases are handled here.
%s format strings allow for padding and alignment, but their behaviour is very different from fstrings.
%20s will pad a string to 20 characters, and right align %-20s will pad a string to 20 characters, and left align
This behaviour is carried over from the C *printf() functions.
This patch adds the ability to properly convert these to fstrings, using the correct alignment markers.
The feature is gated behind aggressive mode for now.
The earlier code did already convert %20s, but changed the alignment (the resulting fstring would be left aligned instead of right), and did not understand %-20s at all.