-
Notifications
You must be signed in to change notification settings - Fork 33
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
Handle %s strings with length and alignment #194
base: master
Are you sure you want to change the base?
Changes from all commits
4acbc3d
b4e19c6
00b6bc4
cd146b0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,6 +40,24 @@ def test_call(state: State): | |
|
||
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 | ||
Comment on lines
41
to
+56
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These two tests are exactly the same? lets delete one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes. |
||
|
||
|
||
def test_string_specific_len_left_aligned(state: State): | ||
s_in = """'%-5s' % CLASS_NAMES[labels[j]]""" | ||
s_expected = """f'{CLASS_NAMES[labels[j]]:5}'""" | ||
|
||
state.aggressive = True | ||
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.