Add basic dictionary impl #92
Conversation
There was a problem hiding this comment.
Thanks a lot, this is very valuable work to get initial Dictionary support! 🙂
Added some comments. As a small nitpick, could you end documentation lines with a full stop, even if they are short? E.g:
/// Constructs an empty `Dictionary`
pub fn new() -> Self {...}becomes
/// Constructs an empty `Dictionary`.
pub fn new() -> Self {...}
|
bors try |
tryBuild failed: |
|
|
Floats aren't |
lilizoey
force-pushed
the
feature/dictionary
branch
from
aa1b50f
to
3786a0a
Compare
There was a problem hiding this comment.
Thanks for the update!
Instead of impl ToVariant, can you use explicit generic arguments? That allows the caller to specify them, to dis-ambiguate the call if needed.
So instead of
pub fn get(&self, key: impl ToVariant) -> Option<Variant>you'd have
pub fn get<K: ToVariant>(&self, key: K) -> Option<Variant>
godot-core/src/builtin/dictionary.rs
Outdated
| /// Creates a new dictionary with the given keys and values, the syntax mirrors | ||
| /// godot's dictionary creation syntax. | ||
| /// | ||
| /// Any value can be used as a key, but to use an expression you need to surround it | ||
| /// in `()` or `{}` | ||
| /// | ||
| /// Example | ||
| /// ```rust, no_run | ||
| /// # #[macro_use] extern crate godot_core; | ||
| /// # fn main() { | ||
| /// let key = "my_key"; | ||
| /// let d = dict! { | ||
| /// "key1": 10, | ||
| /// "another": Variant::nil(), | ||
| /// key: true, | ||
| /// (1 + 2): "final", | ||
| /// } | ||
| /// # } | ||
| /// ``` |
There was a problem hiding this comment.
Nitpicks:
- uppercase "Godot"
- missing colon after "Example"
- missing full stops at end of sentences
```rust, no_run->```no_run(Rust is implied)
Maybe check this in other places as well.
There was a problem hiding this comment.
i uppercased "Godot" in all the documentation i could find. i found two places i forgot a full stop too, hopefully i didnt miss anything else.
|
bors try |
tryBuild failed: |
Run rustfmt
lilizoey
force-pushed
the
feature/dictionary
branch
from
c6d87c9
to
752bdac
Compare
| /// Convert the keys of this dictionary to a strongly typed rust `HashSet`. If the | ||
| /// conversion fails for any key, an error is returned. | ||
| impl<K: FromVariant + Eq + std::hash::Hash> TryFrom<&Dictionary> for HashSet<K> { | ||
| type Error = VariantConversionError; | ||
|
|
||
| fn try_from(dictionary: &Dictionary) -> Result<Self, Self::Error> { | ||
| // TODO: try to panic or something if modified while iterating | ||
| // Though probably better to fix when implementing iteration proper | ||
| dictionary | ||
| .keys() | ||
| .iter_shared() | ||
| .map(|key| K::try_from_variant(&key)) | ||
| .collect() | ||
| } | ||
| } |
There was a problem hiding this comment.
This is OK for now, but might later be removed, once we have full iterator support.
Reason: maps and sets are different data structures, so I'm not sure if a direct From/TryFrom conversion is the right approach. For example, there is no such conversion between HashMap and HashSet in the standard library.
Instead, we can add iterators over key-value pairs, keys and values. It should then be relatively simple to collect() them into their respective data structures, and we can make this API more versatile and agnostic of concrete types like HashSet.
tryBuild failed: |
Hopefully the `from_sys_init` thing used in array will stop the `duplicate` crash
lilizoey
force-pushed
the
feature/dictionary
branch
from
e354378
to
2c2f924
Compare
|
Build succeeded: |
|
Manually re-merged in 188aa69, as I forgot to squash 🙃 |
Add basic dictionary impl with:
FromforHashMapandHashSet(with true as the values)FromIteratorfor(K,V)dict!macro to easily make dictionaries godot-stylemissing:
get_mut(and other_mutfunctions)key = valuesyntax fordict!macroThe added
PartialEq => array_operator_equalimpl inArrayis there to simplify a test, but can be removed. But i think it's gonna be added eventually anyway.I'm also not entirely sure about the safety of using dictionaries, my guess is that it's the same as for array though.