Add Message.python_brace_format#1169
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1169 +/- ##
==========================================
+ Coverage 91.31% 91.37% +0.05%
==========================================
Files 27 27
Lines 4654 4672 +18
==========================================
+ Hits 4250 4269 +19
+ Misses 404 403 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
babel/messages/catalog.py
Outdated
| def _has_python_brace_format(string: str) -> bool: | ||
| fmt = Formatter() | ||
| try: | ||
| parsed = list(fmt.parse(string)) | ||
| except ValueError: | ||
| return False | ||
| return any(True for _, field_name, *_ in parsed if field_name is not None) |
There was a problem hiding this comment.
How about
| def _has_python_brace_format(string: str) -> bool: | |
| fmt = Formatter() | |
| try: | |
| parsed = list(fmt.parse(string)) | |
| except ValueError: | |
| return False | |
| return any(True for _, field_name, *_ in parsed if field_name is not None) | |
| def _has_python_brace_format(string: str) -> bool: | |
| if "{" not in string: | |
| return False | |
| fmt = Formatter() | |
| try: | |
| # `fmt.parse` returns 3-or-4-tuples of the form | |
| # `(literal_text, field_name, format_spec, conversion)`; | |
| # if `field_name` is set, this smells like brace format | |
| return any(t[1] is not None for t in fmt.parse(string)) | |
| except ValueError: | |
| return False |
?
- fast path first
- no reason to gather the result to a list first that I can think of
There was a problem hiding this comment.
Agreed on both, there is no need to convert the result to a list, it just didn't occur to me when I wrote it 😅
There was a problem hiding this comment.
Actually, there is a subtle difference. Consider the string {} {. If you try to format it, you'll get a ValueError because of the unclosed second brace which makes it invalid. However, _has_python_brace_format will return True because the any short-circuits before the parser gets to the invalid brace.
This might be a bit surprising. Perhaps we should only return True if the whole string is valid?
There was a problem hiding this comment.
Mm, fair! Maybe the any could be rewritten as a not all(...) (with a suitable comment above it to explain why the strange construction!) so we do always parse the entire thing?
There was a problem hiding this comment.
I went for a loop in the end because I find it to be a bit easier to read compared to not all(...) but I'm happy to change it if you prefer!
Co-authored-by: Aarni Koskela <akx@iki.fi>
akx
left a comment
akx
left a comment
There was a problem hiding this comment.
LGTM. A verbose for loop with comments is better than a hard-to-comprehend any()/all()/not all() expression!
2 participants
Relevant issue: #333
Part 1 of adding support for checking f-string parameters. This simply adds a new property on the
Messageobject analogous topython_formatwhich sets the corresponding flag. In a followup PR I'm going to add a checker for the f-string format (reusing my implementation from #1168)@akx (I can't ask for a review so I'm tagging you ;))