Skip to content

Conversation

@loudblow
Copy link

Resolving issue with accessing message attributes via FluentLocalization.format_value("message.attribute")

Connected issues:
#209

Copy link
Member

@eemeli eemeli left a comment

Choose a reason for hiding this comment

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

Extending format_value() like this would be rather misleading, as the added functionality is specifically about formatting a message attribute, not its value.

Instead, I think we should add a new method format_message(), which takes the same arguments as format_value(), but returns a tuple of the formatted value (or None, if there is no value), together with a dict of all of the formatted attributes, keyed by their names.

This connects with another Fluent design choice: attributes should not be used separately, but only as a part of the whole message.

Also, we do need to include tests and a documentation update when making an API change like this.

@loudblow
Copy link
Author

Sounds fair enough, I'll try to implement that soon. Anyway, I think that added functionality could be added too (with tests and docs, of course). According to the function name it should format any kind of values, even it is an attribute of a message, because attributes also have their own values.

@loudblow
Copy link
Author

According to the function name it should format any kind of values, even it is an attribute of a message, because attributes also have their own values.

Never mind. Pattern resolving works not as I thought, so I refuse the idea. Just will implement what you've mentioned above

@loudblow loudblow requested a review from eemeli November 27, 2025 20:02
@loudblow loudblow changed the title fluent.runtime: Add attribute formatting functionality in FluentLocalization runtime: add attribute formatting functionality in FluentLocalization Nov 27, 2025
Copy link
Member

@eemeli eemeli left a comment

Choose a reason for hiding this comment

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

Looks promising! See inline for detail-y comments.

Comment on lines +60 to +63
for bundle in self._bundles():
if not bundle.has_message(msg_id):
continue
msg = bundle.get_message(msg_id)
Copy link
Member

Choose a reason for hiding this comment

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

Why not something like this?

Suggested change
for bundle in self._bundles():
if not bundle.has_message(msg_id):
continue
msg = bundle.get_message(msg_id)
msg = next((
bundle.get_message(msg_id)
for bundle in self._bundles()
if bundle.has_message(msg_id)
), None)

Copy link
Author

Choose a reason for hiding this comment

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

The first reason is consistency, format_value also uses a for loop. The second reason is simplicity: the code looks linear and less bloated to those seeing it for the first time.

Comment on lines 79 to 84
return FormattedMessage(
# Never FluentNone when format_pattern called externally
cast(str, val),
formatted_attrs if formatted_attrs else {},
)
return FormattedMessage(msg_id, {})
Copy link
Member

Choose a reason for hiding this comment

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

Let's just return a (val, formatted_attrs) tuple rather than defining a new class.

Copy link
Author

Choose a reason for hiding this comment

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

This is a regular tuple with attributes. Some users may prefer to pass a single DTO in their code instead of passing the message and attributes separately. Using fmt_msg.message and fmt_msg.attributes seems more intuitive than fmt_msg[0] and fmt_msg[1]

@loudblow loudblow requested a review from eemeli November 28, 2025 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants