Array.alt concats in reverse order
mlms13 opened this issue · 7 comments
Array.Alt.alt([|0, 1|], [|2, 3|]) == [|2, 3, 0, 1|];This is because the definition is equal to Js.Array.concat which combines arrays in this order:
https://github.com/Risto-Stevcev/bs-abstract/blob/master/src/implementations/Array.re#L58
This seems unexpected to me, but I don't think it actually breaks any laws (it's still associative and distributive). If the current behavior isn't intended, I'm happy to submit a pull request.
It looks like it was written that way so that you can do something like this:
let _ =
[|2;3;4|]
|> Js.Array.concat [|5;6;7|]
|> Js.logBut it's different from the way it's implemented in javascript and purescript. Yeah I think you're right, we should probably change the current behavior so it's more intuitive and people can still use |. for that. I'll accept a PR 🙂
Just a note:
List.Alt.alt([0, 1], [2, 3]) == [0, 1, 2, 3]
I think Array should work the same as List.
I was thinking about this some more, and I'm thinking maybe it's not a bad idea to create some Belt interfaces of all of these things for "target first" (Belt.Monoid, Belt.Functor, etc). That way we'll have abstractions that cover the Belt API and we can then use the Belt version for this instead of introducing a breaking change.
What do you guys think?
I hadn't thought about the fact that usually the "significant" argument comes last, and in the case of alt, you could definitely argue that most significant array is the one being concat'd onto.
This initially came up because it feels especially wrong with the <|> infix:
let (<|>) = BsAbstract.Array.Infix.(<|>);
let combined = [| 0, 1, 2 |] <|> [| 3, 4, 5 |]; // [| 3, 4, 5, 0, 1, 2 |]If you want to make significant-data-first modules for better compatibility with Belt, I won't try to stop you, but I also doubt I'd use them. 🙂 I generally prefer significant-data-last, but mostly I want typeclass-inspired functions that work like the PureScript equivalents, and alt is the one outlier (that I've seen so far).
It looks like implementations/Array.re uses a mix of functions from ArrayLabels and BuckleScript's Js.Array, and in this case, ArrayLabels.append would concat the arrays in the expected order, even though ArrayLabels is usually significant-data-last.
Yeah that's true, the operators really should be intuitive and compatible with purescript/haskell semantics. I don't really use Belt much either, but I'll still probably add some Belt interfaces eventually since it's used by Relude and some people like it.