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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Sajjon
Copy link

@Sajjon Sajjon commented Jul 3, 2024

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:

  • single
  • only
  • with
  • Or from_just, from_single, from_only
  • Or with_just, with_single, with_only

But I deemed just most convenient and yet still clear.

@Sajjon Sajjon force-pushed the feature/new_ctor_single_item branch from d29c2be to c2fd40b Compare July 3, 2024 10:49
@Sajjon Sajjon changed the title Add just: new constructor to IndexSet and IndexMap respectively Add just: new constructor to IndexSet and IndexMap respectively Jul 3, 2024
@cuviper
Copy link
Member

cuviper commented Jul 3, 2024

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
Copy link
Author

Sajjon commented Jul 3, 2024

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
Copy link
Member

cuviper commented Jul 8, 2024

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

@cuviper cuviper added the waiting-for-std Changes waiting for stabilization in the standard library, so we can match the API. label Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-for-std Changes waiting for stabilization in the standard library, so we can match the API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants