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

List API is inconsistent with other modules #192

Open
cknitt opened this issue Feb 15, 2024 · 4 comments
Open

List API is inconsistent with other modules #192

cknitt opened this issue Feb 15, 2024 · 4 comments

Comments

@cknitt
Copy link
Collaborator

cknitt commented Feb 15, 2024

This is for historical reasons (the List module is copied from Belt).

For example:

  • List: let forEach: (t<'a>, 'a => 'b) => unit
  • Others: let forEach: (t<'a>, 'a => unit) => unit

or

  • List: let mapWithIndex: (t<'a>, (int, 'a) => 'b) => t<'b>
  • Others: let mapWithIndex: (t<'a>, ('a, int) => 'b) => t<'b>

or

  • List: getBy
  • Others: find
@zth
Copy link
Collaborator

zth commented Feb 15, 2024

@cknitt I think we can change it to make it consistent. Good to do before Core goes into the compiler. What do you think?

@cknitt
Copy link
Collaborator Author

cknitt commented Feb 15, 2024

Yes, absolutely, I agree we should make it consistent before Core goes into the compiler.
I can make a PR for that some time in the next days.

@cknitt
Copy link
Collaborator Author

cknitt commented Feb 17, 2024

@zth See #195.

But there are still some open questions:

  1. List has type t<'a> = list<'a>. Array doesn't have a t<'a> defined, and t<'a> was also removed from Result recently. If the idea is to define a t<'a> only where necessary, then it should be removed from List, too?
  2. List has toShuffled, but sort, reverse etc. all without to. As this is an immutable data structure, we do not need the distinction between shuffle and toShuffled like in Array. So remove the to?
  3. List.concatMany signature is different from Array.concatMany. Personally I find the Array.concatMany signature weird anyway (and different from what it was in Belt.Array) and would actually change it there.
  4. getAssoc, hasAssoc, removeAssoc, setAssoc: These serve for abusing a list as a map. Given that we have both the mutable JS Map and the immutable Belt.Map available in ReScript, should we really keep these?
  5. Map and Set have has and size, Array has includes and length. What should List have?

@cknitt
Copy link
Collaborator Author

cknitt commented Feb 17, 2024

Personally I find the Array.concatMany signature weird anyway (and different from what it was in Belt.Array) and would actually change it there.

Which would then be the same as Array.flat. Hmmm. So not sure what to do about the different concatManys.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants