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

Change FluentArgs key type to String. #179

Closed
XAMPPRocky opened this issue May 31, 2020 · 5 comments
Closed

Change FluentArgs key type to String. #179

XAMPPRocky opened this issue May 31, 2020 · 5 comments
Labels
crate:fluent-bundle Issues related to fluent-bundle crate design-decision Issues pending design decision, usually Rust specific enhancement help wanted We need help making decisions or writing PRs for this. question
Milestone

Comments

@XAMPPRocky
Copy link

XAMPPRocky commented May 31, 2020

Currently FluentArgs is a HashMap<&str, FluentValue>, which is okay when you're using str literals as keys, however when you're constructing your map dynamically you don't have a HashMap<&str, V> only a String. Currently you have to construct a new HashMap that takes the key by reference and clones the FluentValue. It would easier for users and more appropriate if it was HashMap<String, FluentValue>.

// Current
pub type FluentArgs<'args> = HashMap<&'args str, FluentValue<'args>>;
// Proposed
pub type FluentArgs<'args> = HashMap<String, FluentValue<'args>>;
@zbraniecki
Copy link
Collaborator

Hmm, I'm torn on this. I'm wondering if there's a way to handle both scenarios without having to give up on the former.

@zbraniecki
Copy link
Collaborator

See #94 for the motivation for current design.

@zbraniecki zbraniecki added design-decision Issues pending design decision, usually Rust specific enhancement crate:fluent-bundle Issues related to fluent-bundle crate help wanted We need help making decisions or writing PRs for this. question labels Jun 16, 2020
@XAMPPRocky
Copy link
Author

XAMPPRocky commented Jun 16, 2020

@zbraniecki AFAIK the best way to handle this would be Generic Associated Types but that isn't currently available. I don't know what the best workaround would be but with the current API I can actually be allocating more since I have to construct a new map with each key by reference and each value by value versus just allocating a string each time. If I wanted to reduce allocations with Fluent I could do that on the user side with memoization for the argument map but I actually can't use memoization on FluentArgs currently because memoization requires owning the data in some way.

@zbraniecki
Copy link
Collaborator

@XAMPPRocky can you check if the current master branch solved it? I switched FluentArgs to accept Cow which hopefully solves it.

@zbraniecki zbraniecki added this to the 0.13 milestone Sep 18, 2020
@XAMPPRocky
Copy link
Author

Yeah Cow works for me. Thanks for working on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate:fluent-bundle Issues related to fluent-bundle crate design-decision Issues pending design decision, usually Rust specific enhancement help wanted We need help making decisions or writing PRs for this. question
Projects
None yet
Development

No branches or pull requests

2 participants