Skip to content

Add Message.python_brace_format #1169

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jan 16, 2025
Merged

Add Message.python_brace_format #1169

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
08512c7
Add support for python-brace-format
tomasr8 Jan 12, 2025
fe8a756
Optimize checking for format parameters
tomasr8 Jan 13, 2025
2529cff
Fix tests
tomasr8 Jan 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions babel/messages/catalog.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
from difflib import SequenceMatcher
from email import message_from_string
from heapq import nlargest
from string import Formatter
from typing import TYPE_CHECKING

from babel import __version__ as VERSION
Expand Down Expand Up @@ -69,6 +70,15 @@ def get_close_matches(word, possibilities, n=3, cutoff=0.6):
''', re.VERBOSE)


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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about

Suggested change
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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😅

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!



def _parse_datetime_header(value: str) -> datetime.datetime:
match = re.match(r'^(?P<datetime>.*?)(?P<tzoffset>[+-]\d{4})?$', value)

Expand Down Expand Up @@ -140,6 +150,10 @@ def __init__(
self.flags.add('python-format')
else:
self.flags.discard('python-format')
if id and self.python_brace_format:
self.flags.add('python-brace-format')
else:
self.flags.discard('python-brace-format')
self.auto_comments = list(distinct(auto_comments))
self.user_comments = list(distinct(user_comments))
if isinstance(previous_id, str):
Expand Down Expand Up @@ -252,6 +266,21 @@ def python_format(self) -> bool:
ids = [ids]
return any(PYTHON_FORMAT.search(id) for id in ids)

@property
def python_brace_format(self) -> bool:
"""Whether the message contains Python f-string parameters.

>>> Message('Hello, {name}!').python_brace_format
True
>>> Message(('One apple', '{count} apples')).python_brace_format
True

:type: `bool`"""
ids = self.id
if not isinstance(ids, (list, tuple)):
ids = [ids]
return any(_has_python_brace_format(id) for id in ids)


class TranslationError(Exception):
"""Exception thrown by translation checkers when invalid message
Expand Down
27 changes: 27 additions & 0 deletions tests/messages/test_catalog.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,24 @@ def test_python_format(self):
assert catalog.PYTHON_FORMAT.search('foo %(name)*.*f')
assert catalog.PYTHON_FORMAT.search('foo %()s')

def test_python_brace_format(self):
assert not catalog._has_python_brace_format('')
assert not catalog._has_python_brace_format('foo')
assert not catalog._has_python_brace_format('{')
assert not catalog._has_python_brace_format('}')
assert not catalog._has_python_brace_format('{} {')
assert not catalog._has_python_brace_format('{{}}')
assert catalog._has_python_brace_format('{}')
assert catalog._has_python_brace_format('foo {name}')
assert catalog._has_python_brace_format('foo {name!s}')
assert catalog._has_python_brace_format('foo {name!r}')
assert catalog._has_python_brace_format('foo {name!a}')
assert catalog._has_python_brace_format('foo {name!r:10}')
assert catalog._has_python_brace_format('foo {name!r:10.2}')
assert catalog._has_python_brace_format('foo {name!r:10.2f}')
assert catalog._has_python_brace_format('foo {name!r:10.2f} {name!r:10.2f}')
assert catalog._has_python_brace_format('foo {name!r:10.2f=}')

def test_translator_comments(self):
mess = catalog.Message('foo', user_comments=['Comment About `foo`'])
assert mess.user_comments == ['Comment About `foo`']
Expand Down Expand Up @@ -342,10 +360,19 @@ def test_message_pluralizable():


def test_message_python_format():
assert not catalog.Message('foo').python_format
assert not catalog.Message(('foo', 'foo')).python_format
assert catalog.Message('foo %(name)s bar').python_format
assert catalog.Message(('foo %(name)s', 'foo %(name)s')).python_format


def test_message_python_brace_format():
assert not catalog.Message('foo').python_brace_format
assert not catalog.Message(('foo', 'foo')).python_brace_format
assert catalog.Message('foo {name} bar').python_brace_format
assert catalog.Message(('foo {name}', 'foo {name}')).python_brace_format


def test_catalog():
cat = catalog.Catalog(project='Foobar', version='1.0',
copyright_holder='Foo Company')
Expand Down
Loading