Some comments on the Array module

[glennsl]

• from is unsafe (not soundly typed), but not marked as such
• So is fromWithMap. It's also misleadingly named, suggesting it takes a Map data structure as an additional argument, when in fact it wants a map function. The latter goes for all *WithMap functions, but I won't repeat myself for every one of them. A better naming scheme for these might be mapFrom*?
• *WithIndex is a really common and quite verbose pattern. I would suggest special casing this and use just an i suffix for this, e.g. mapi instead of mapWithIndex. I think it's common enough that this makes sense.
• indexOfFrom and lastIndexOfFrom are not very intuitively named. Perhaps indexOfStartingFrom would be better?
• getSymbol seems like it would return a symbol, rather than what is pointed to by a symbol. getBySymbol and setBySymbol might be better names.

[cknitt]

• from is unsafe (not soundly typed), but not marked as such

Right. Not sure why we have the unsafe "from" functions anyway. Can't we always use arrayLike, and if people really want to be unsafe for some reason, then they can cast to an arrayLike using Obj.magic? BTW I think we should define Array.t and Array.arrayLike:

type t<'a> = array<'a>
type arrayLike<'a> = Js.Array2.array_like<'a>

@val external from: arrayLike<'a> => t<'a> = "Array.from"
@val external fromWithMap: (arrayLike<'a>, 'a => 'b) => t<'b> = "Array.from"

• So is fromWithMap. It's also misleadingly named, suggesting it takes a Map data structure as an additional argument, when in fact it wants a map function. The latter goes for all *WithMap functions, but I won't repeat myself for every one of them. A better naming scheme for these might be mapFrom*?

Hmm, I don't know, it is a version of the JS Array.from method, so it makes sense to call it fromXxx, and we have that fromXxx naming pattern elsewhere, too (Float.fromInt etc.).

• *WithIndex is a really common and quite verbose pattern. I would suggest special casing this and use just an i suffix for this, e.g. mapi instead of mapWithIndex. I think it's common enough that this makes sense.

IMHO this would be inconsistent with the naming in the rest of the library which avoids such abbreviations. To me, this would feel more like "the OCaml way" than "the ReScript way".

[glennsl]

Right. Not sure why we have the unsafe "from" functions anyway. Can't we always use arrayLike, and if people really want to be unsafe for some reason, then they can cast to an arrayLike using Obj.magic?

I agree. I don't think this library needs to exhaustively cover the entire JS API surface either.

Hmm, I don't know, it is a version of the JS Array.from method, so it makes sense to call it fromXxx, and we have that fromXxx naming pattern elsewhere, too (Float.fromInt etc.).

So there are two rules that sort of conflict here already. One is that it's an established rescript convention to use fromXX for conversion constructor functions. The other is that bindings ought to start with the same name as the name of the function it binds to.

I don't think the first rule applies all that much, since from functions typically have only one argument. This has two, and it behaves differently since it applies the second argument to the first during the conversion.

The other rule also has quite a few exceptions already. Just in the Array module, for example, there's copy for slice and copyAllWithin for copyWithin.

It is a downside that mapFrom* can be confused for a variant of the map function though. But I think that can be mitigated by placing it alongside the other constructor functions and having a clear docstring.

IMHO this would be inconsistent with the naming in the rest of the library which avoids such abbreviations. To me, this would feel more like "the OCaml way" than "the ReScript way".

I think I was actually the originator of "the ReScript way", from before it was even "the BuckleScript way", and I can tell you that it wasn't as well thought out and designed as you might think it was. Things were mostly just haphazardly added onto what was already there while trying to strike some balance between consistency, familiarity and intuitiveness. And while I do think this is a good fallback rule, it's also one I think should have plenty of exceptions since it's quite verbose. And indeed there are plenty of exceptions to it already. Just in the Array module, for examplethere'scopyWithinToEndinstead ofcopyWithinWithTargetAndEnd, pushManyinstead ofpushWithArray, and joinWithinstead ofjoinWithString`.

There are also several similar abbreviations in use already, for example U, Opt and Exn .

But this naming is also inconsistent with the more general rule, which tends to describe additional function arguments. Whereas in this case Index isn't an additional argument to the function itself, but an additional argument to a callback function that is an argument to the named function. I don't think there's really an intentionally established convention for this. And I don't know if there should be either.

Still, I can understand that the i suffix can seem too abbreviated, but I'm struggling to come up with a better naming scheme and I think it's common enough that it's OK.

A more out-of-the-box alternative might be to remove these variant entirely, and just have the standard functions always pass the index. That means you'd have to ignore it when it's not used (e.g. arr->Array.map((el, _) => el.foo)), but I think that would be fine too.

[zth]

Partly covered in the docstrings PR: #78

[jmagaram]

I agree WithIndex is verbose. findIndexWithIndex is the worst. In functions like someWithIndex and everyWithIndex the WithIndex part becomes more important than the core function name. F# and OCaml use the letter i abbreviation. I wouldn't make the index part required in a callback since I think most of the time it isn't needed. It's just so nice to write those callbacks as x => rather than (x,_) =>