search

projectfluent / fluent-rs

Rust implementation of Project Fluent
https://projectfluent.org
Apache License 2.0
1.05k stars 95 forks source link

Change `FluentArgs` key type to `String`. #179

Closed XAMPPRocky closed 3 years ago

XAMPPRocky commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

See #94 for the motivation for current design.

XAMPPRocky commented 4 years ago

@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 commented 3 years ago

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

XAMPPRocky commented 3 years ago

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