Skip to content
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

Allow customizing argument resolver #304

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

JasperDeSutter
Copy link
Contributor

The main value add for a custom argument resolver is not needing to put arguments into a FluentArgs, which has no type safety. See the typesafe_messages example that demonstrates this use case.
The first commit optimizes FluentArg by not storing keys as Cows, but as regular &str instead. This reduces a small overhead in data size and arg lookup performance.

@gregtatum gregtatum self-requested a review December 19, 2022 16:02
Copy link
Member

@gregtatum gregtatum left a comment

Choose a reason for hiding this comment

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

This all seems reasonable to me, and allows for more programmatic integration with a Rust codebase. Since this is a design change I would like to get another reviewer on this to double check.

@nordzilla Would you mind looking at this briefly? The last commit includes an example that shows how to use it which I would appreciate your opinion on. You can also see the implementation of ArgumentResolver in fluent-bundle/src/resolver/scope.rs. You can track down the calls of it by searching for .resolve. I don't think you need to look at the first commit, since that's more of an implementation detail.

fluent-bundle/src/resolver/scope.rs Show resolved Hide resolved
fluent-bundle/examples/typesafe_messages.rs Show resolved Hide resolved
@gregtatum
Copy link
Member

Oh and also thanks for the PR :)

alerque

This comment was marked as duplicate.

Copy link
Collaborator

@alerque alerque left a comment

Choose a reason for hiding this comment

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

I rebased this just to fix the commit messages and move the doc fixup commit into the one it was fixing. I did not resolve the merge conflict that has come up since this was contributed. @JasperDeSutter is there any chance you are still around and could have a look at this? This project is in a little better position to move on with contributions now (see #347 for background).

@JasperDeSutter
Copy link
Contributor Author

Great to see this project more alive again! I'll pick up my PRs that are still open, though it has been a while so it might take some time to get them all updated.

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.

3 participants