Update logical line rules for the new f-string tokens - #7690
Merged
Conversation
This was referenced Sep 28, 2023
Member
Author
1 similar comment
Member
Author
3 tasks
dhruvmanila
marked this pull request as ready for review
September 28, 2023 04:06
| } | ||
| _ => {} | ||
| } | ||
| if let Some(symbol) = BracketOrPunctuation::from_kind(kind) { |
Member
There was a problem hiding this comment.
Should we just skip all of this when we're inside an f-string?
Member
There was a problem hiding this comment.
Oh, or actually, we do want to catch some errors within f-strings -- is that right?
Member
Author
There was a problem hiding this comment.
Removing whitespaces before the punctuations should be fine. It's only when they're removed after when the semantics changes.
So, f"{foo :.3f}" and f"{foo:.3f}" are equivalent.
Member
Author
There was a problem hiding this comment.
Oh, or actually, we do want to catch some errors within f-strings -- is that right?
Yes
CodSpeed Performance ReportMerging #7690 will degrade performances by 2.28%Comparing Summary
Benchmarks breakdown
|
| let needs_space = if kind == TokenKind::Equal && parens > 0 { | ||
| // Allow keyword args or defaults: foo(bar=None). | ||
| let needs_space = if kind == TokenKind::Equal && (parens > 0 || fstrings > 0) { | ||
| // Allow keyword args, defaults: foo(bar=None) and f-strings: f'{foo=}' |
Member
There was a problem hiding this comment.
Again, should we just have something like:
if fstrings > 0 {
continue;
}Or is it important that we run this body logic?
Member
There was a problem hiding this comment.
Oh, so we're trying to catch some of these errors within f-strings, but not all -- is that right?
Member
Author
There was a problem hiding this comment.
Yes, it's only when the equal sign is present which in f-string context will be used as debug expression. So, f"{x=} and f"{x = }" gives different output.
Contributor
PR Check ResultsEcosystemℹ️ ecosystem check detected changes. (+0, -105, 0 error(s)) rotki (+0, -37)
- rotkehlchen/accounting/structures/base.py:148:37: E225 Missing whitespace around operator - rotkehlchen/accounting/structures/base.py:149:35: E225 Missing whitespace around operator - rotkehlchen/accounting/structures/base.py:150:30: E225 Missing whitespace around operator - rotkehlchen/accounting/structures/base.py:151:29: E225 Missing whitespace around operator - rotkehlchen/accounting/structures/base.py:152:31: E225 Missing whitespace around operator - rotkehlchen/accounting/structures/base.py:153:34: E225 Missing whitespace around operator - rotkehlchen/accounting/structures/base.py:154:26: E225 Missing whitespace around operator - rotkehlchen/accounting/structures/base.py:155:28: E225 Missing whitespace around operator - rotkehlchen/accounting/structures/base.py:156:35: E225 Missing whitespace around operator - rotkehlchen/accounting/structures/base.py:157:26: E225 Missing whitespace around operator - rotkehlchen/accounting/structures/base.py:158:31: E225 Missing whitespace around operator - rotkehlchen/accounting/structures/eth2.py:139:58: E225 Missing whitespace around operator - rotkehlchen/accounting/structures/eth2.py:139:77: E225 Missing whitespace around operator - rotkehlchen/accounting/structures/eth2.py:265:139: E225 Missing whitespace around operator - rotkehlchen/accounting/structures/eth2.py:265:53: E225 Missing whitespace around operator - rotkehlchen/accounting/structures/eth2.py:265:72: E225 Missing whitespace around operator - rotkehlchen/accounting/structures/eth2.py:394:55: E225 Missing whitespace around operator - rotkehlchen/accounting/structures/eth2.py:394:74: E225 Missing whitespace around operator - rotkehlchen/accounting/structures/eth2.py:394:91: E225 Missing whitespace around operator - rotkehlchen/accounting/structures/evm_event.py:261:28: E225 Missing whitespace around operator - rotkehlchen/accounting/structures/evm_event.py:262:33: E225 Missing whitespace around operator - rotkehlchen/accounting/structures/evm_event.py:263:28: E225 Missing whitespace around operator - rotkehlchen/accounting/structures/evm_event.py:264:28: E225 Missing whitespace around operator - rotkehlchen/accounting/structures/evm_event.py:265:31: E225 Missing whitespace around operator - rotkehlchen/chain/ethereum/modules/aave/v2/decoder.py:175:126: E231 [*] Missing whitespace after ':' - rotkehlchen/fval.py:159:31: E231 [*] Missing whitespace after ':' - rotkehlchen/fval.py:49:27: E231 [*] Missing whitespace after ':' - rotkehlchen/tests/conftest.py:140:47: E231 [*] Missing whitespace after ':' - rotkehlchen/tests/unit/test_defi_oracles.py:44:102: E225 Missing whitespace around operator - rotkehlchen/tests/unit/test_defi_oracles.py:44:84: E225 Missing whitespace around operator - tools/profiling/cpu.py:12:35: E231 [*] Missing whitespace after ':' - tools/profiling/cpu.py:13:33: E231 [*] Missing whitespace after ':' - tools/profiling/cpu.py:30:36: E231 [*] Missing whitespace after ':' - tools/profiling/graph.py:219:33: E231 [*] Missing whitespace after ':' - tools/profiling/sampler.py:64:30: E231 [*] Missing whitespace after ':' - tools/profiling/sampler.py:64:43: E231 [*] Missing whitespace after ':' - tools/profiling/trace.py:26:28: E231 [*] Missing whitespace after ':' sphinx (+0, -68)
-
- = help: Added missing whitespace after ':'
- |
- | ^ E231
- = help: Added missing whitespace after ':'
- |
- | ^ E231
- | ^ E231
- | ^ E231
- | ^ E231
- | ^ E231
- | ^ E231
- = help: Added missing whitespace after ':'
- |
- | ^ E231
- | ^ E231
- | ^ E231
- | ^ E231
- | ^ E231
- | ^ E231
- 1202 | """Return an RFC 3339 formatted string representing the given timestamp."""
- 1203 | seconds, fraction = divmod(timestamp, 1)
- 1204 | return time.strftime("%Y-%m-%d %H:%M:%S", time.gmtime(seconds)) + f'.{fraction:.3f}'
- 130 | if total <= 0:
- 131 | return _('no translated elements!')
- 132 | return f'{translated / total:.2%}'
- 144 | if not content:
- 145 | return ''
- 146 | return f'{zlib.crc32(content):08x}'
- 22 | weekday_name = _WEEKDAY_NAME[wd]
- 23 | month = _MONTH_NAME[mn]
- 24 | return f'{weekday_name}, {dd:02} {month} {yr:04} {hh:02}:{mm:02}:{ss:02} GMT'
- 282 | res = r"%.5f\linewidth" % (amount_float / 100.0)
- 283 | else:
- 284 | res = f"{amount_float:.5f}{unit}"
- 285 | return res
- 293 | table.append([
- 294 | 'TOTAL',
- 295 | f'{100 * len(all_documented_objects) / len(all_objects):.2f}%',
- 296 | f'{len(all_objects) - len(all_documented_objects)}',
- 297 | ])
- 341 | c = f'
|
charliermarsh
approved these changes
Sep 28, 2023
charliermarsh
left a comment
Member
There was a problem hiding this comment.
This looks good to me -- lets just make sure we're setting prev_token in any of those continue blocks.
Member
Author
Thanks! I'll look at that in a bit |
Member
|
How did you identify this specific subset of logical line rules? Did they show up in the ecosystem CI on the PEP 701 PR? |
Member
Author
Yes, then I thought of checking out all the relevant operators and trivia tokens specific to f-string usages. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR updates the logical line rules for the new f-string tokens.
Specifically, it tries to ignore the rules for which it might be hard to differentiate between different usages inside f-strings. For some tokens, the whitespace addition or removal could change the semantics so they're ignored for every scenario inside f-string.
E225for the=operator. For example,f"{x=}"we don't want to add whitespace as the output will then be different than what the user indended.E231for the:operator. For example,f"{x:.3f}"we don't want to add whitespace as the output will be different:E201,E202for{and}tokens. The double braces are used as an escape mechanism to include the raw character (f"{{foo}}") but if a dictionary is being used then the whitespace is required (f"{ {'a': 1} }").Test Plan
Add new test cases for f-strings along with nested f-strings and update the snapshots.