Use IndexSet instead of Vec for codepoints

[cmyr]

This ensures that codepoints are unique, but also respects the original order in which they appear in the file.

This is an alternative to #268. Although it brings in a dependency, it leads to a much simpler and more efficient implementation overall.

Thoughts @madig ?

[RickyDaMa]

indexmap is already pulled in as a transitive dependency, so this seems like a cheap solution to the problem while rocking the boat as little as possible

All it may end up doing is meaning users of the library have to pull in indexmap themselves non-transitively to be able to interact with the data type, depending on what they're doing with it

This creates a case for keeping codepoints private and exposing methods to interact with it, as then crate consumers don't have to care about the underlying data types being used

[madig]

The readme says:

Preserves insertion order as long as you don't call .remove().

Hm :) Are we gonna let that stop our OCD?

[madig]

Otherwise, I'm ok with pulling it in and exposing it directly in the API. I don't like making delegation functions...

[RickyDaMa]

I'm ok with pulling it in and exposing it directly in the API

Personally I'd rather not, however it is a common dependency so definitely not the end of the world.

As far as I can tell from their documentation, only remove changes item order, not something like retain

[cmyr]

So I'm okay going either way with this. Fwiw it would be easy enough to modify the contents without needing to have the crate in scope, e.g.

my_glyph.codepoints.clear();
my_glyph.codepoints.extend(['a', 'b', 'c']);

but I think maybe not having it be public API is a strictly better design, so I'll go ahead and do that.

[cmyr]

Okay, I've updated this with a new custom Codepoints type, wrapping an indexset.

[RickyDaMa]

I'd remove the [] here, at best they go through to the rustdoc as is, or at worst they break your link.
While I'm on this line of code, it's "usage" 😉

[RickyDaMa]

Looks good to me! The only this to bear in mind is that we are still exposing the iterator type from indexmap, but I don't anticipate that causing any issues given that you're always going to be interacting with it through standard APIs

You could also trivially make a borrowing iterator that returns owned chars, since char is Copy, e.g. by doing:

fn iter(&self) -> impl Iterator<Item = char> {
    self.0.iter().copied()
}

[cmyr]

@RickyDaMa unfortunately I can't use impl Iterator with the IntoIterator trait. :(

[RickyDaMa]

Yeah in IntoIterator I knew you wouldn't be able to, hence not proposing a solution there, more just making the observation. I would expect it to be fine though

Maybe in some future version of the Rust compiler 😄