indexmap-rs / indexmap

A hash table with consistent order and fast iteration; access items by key or sequence index
https://docs.rs/indexmap/
Other
1.67k stars 150 forks source link

Add `just`: new constructor to `IndexSet` and `IndexMap` respectively #331

Open Sajjon opened 2 months ago

Sajjon commented 2 months ago

Adding a new constructor to IndexMap and IndexSet respectively: just accepting a single element.

Rationale

The indexset! macro is great, but I did not know about it until just now, while implementing this PR. I think a named constructor has a visibility benefit, that merits its existence. I think a common Developer Experience is to discover constructors using code completion, using turbo fish.

Also the docs of indexset type, does not even mention the macro - which ofc can be rectified with an update to the docs....

Constructors are also a crucial building in fuctional paradigms, such as map, here is an example:

assert_eq!(
    "42".parse::<u8>().map(IndexSet::<_>::just).unwrap(),
    indexset!(42)
);

Here we can map the Result<u8, _> to IndexSet<u8> using the new constructor just, and we note that of course this does not work with the indexset! macro:

assert_eq!(
    "42".parse::<u8>().map(indexset).unwrap(), // ‼️ expected value, found macro `indexset`, not a value
    indexset!(42)
);

Alternative

Other alternative names considered instead of just:

But I deemed just most convenient and yet still clear.

cuviper commented 2 months ago

This proposal doesn't rely on unique features of IndexMap/IndexSet, which makes it a case where I would prefer to see an API standardized in HashMap/HashSet first, so we can implement it consistently.

Besides the macros, there are two ways to do this already -- using From<[(K, V); N]> or From<[T; N]> (for S = RandomState), or using FromIterator with arrays, options, iter::once, etc. (for any S). You chose the latter for your just implementation, but note that the open S is what forces you to use more explicit turbofish hints like ::<_>, where you're still using type inference but nudging it to the default S by omission.

In general, I am skeptical that single-item maps/sets are important enough to deserve a dedicated constructor, when From and FromIterator implementations already work for arbitrary lengths.

Sajjon commented 2 months ago

This proposal doesn't rely on unique features of IndexMap/IndexSet, which makes it a case where I would prefer to see an API standardized in HashMap/HashSet first, so we can implement it consistently.

Hehe, I was hoping to set a precedent with this crate, and then do a PR into Rust lang :)

Maybe it is my specific coding style, of domain, but I oftentimes find myself in need of this ctor.

Did you not think the argument of discoverability had merit?

cuviper commented 2 months ago

Hehe, I was hoping to set a precedent with this crate, and then do a PR into Rust lang :)

Sorry, kind of a chicken-and-egg situation there, since we do want to match std.

What is your background that made just seem discoverable? Based on Haskell Maybe's Just? But I would rather find a way to nudge people toward the more capable From-array, if we can.

With vim-lsp and rust-analyzer, omni-complete of IndexSet::from... does suggest the trait methods to me, but doesn't give any indication of what From types are valid. I wonder if that could be improved. FromIterator is hard/impossible to make suggestions for, since it could be any Iterator with the right Item shape, but that's more obvious from the name.

image