darklang / tablecloth

A standard library with the same API in F#, Rescript and OCaml
https://www.tablecloth.dev
MIT License
513 stars 45 forks source link

Convert Examples into Rescript Syntax #243

Closed Lomand closed 2 years ago

Lomand commented 2 years ago

Code examples converted to Rescript syntax, following these guidelines:

All samples were compiled and executed, and it turns out there are a ton of errors & typos. There are a lot of lists in place of arrays both in native and rescript version, some functions have changed their names and some modules (looks like Tuple was renamed to Tuple2 at some point) do not exist at all.

PS: Changes from #242 should be merged first, most changes here now are duplicated from that PR.

PSPS: Old Tuple naming felt more appropriate, just sain :)

PSPSPS: Originally I was for removing ~f from the Rescript functions, but after looking at examples for so long my mind was changed: ~f does introduce a lot of clarity when you have a lambda function.

Current Status of conversion: ✅ TableclothArray ✅ TableclothBool ✅ TableclothChar ✅ TableclothComparator ✅ TableclothFloat ✅ TableclothFun ✅ TableclothInt ✅ TableclothList ✅ TableclothMap ✅ TableclothOption ✅ TableclothResult ✅ TableclothSet ✅ TableclothString ✅ TableclothTuple2 ✅ TableclothTuple3

Lomand commented 2 years ago

Bool, Char, Float were converted. More typos and uncompilable examples are fixed both in rescript/native. Rescript's Float.fromString was not working as described in examples at all, fixed its implementation, added tests.

pbiggar commented 2 years ago

Wonderful, thanks. This is looking great.

Lomand commented 2 years ago

I've finished converting the rest of the modules. More typos, errors in examples, etc fixed. The project achieved an important milestone: the docs now actually compile :D

I guess now the disclaimer from the README.md about alpha quality software can be dropped, the modules are covered with tests, and the docs are actually good.

Few remarks:

PS: CI fails due to this bug

Lomand commented 2 years ago

One other thing:

The next version of Tablecloth will follow the conventions of the target language (snake_case in Ocaml, camelCase is Rescript). Shouldn't this also affect lists/arrays as the default iterable? Array.groupBy, Result.values, Result.combine, String.split, Map.keys, Map.values return their result as a list.

That seems counterintuitive in Rescript, and if you would not mind, I'll change the Rescript implementation to return arrays.

pbiggar commented 2 years ago

Great, thanks! I'll take a look at your questions this week.

That seems counterintuitive in Rescript, and if you would not mind, I'll change the Rescript implementation to return arrays.

So the issue here is that arrays are mutable. Rescript favors using Arrays because of the intersection with JS, which makes a bit of sense (though I think it's a huge mistake, since immutability is the best thing about FP, but anyway). However, F# and OCaml don't really use arrays because they prefer immutability.

That means if we change the rescript functions to use arrays we end up with significant divergence between the various platforms: it's one thing to have slightly different types or positioning of the t, but differing in immutability changes how you would use a function at all.

Honestly, I don't know what to do here. Half tempted to change arrays to be immutable, as it kinda seems like the only consistent thing to do. But obviously that has problems too.

Any thoughts?

Lomand commented 2 years ago

I think making arrays immutable by default is quite a desirable feature (and also would favorably differentiate Belt and Tablecloth). However, there are still quite a number of scenarios where having mutable arrays is desirable.

To cover both of the use-cases I would suggest dealing with this by:

If you will find such an approach desirable, I'll open an issue with a detailed plan of the implementation.

pbiggar commented 2 years ago

This is a great approach! Reminds me of Belt actually where all the collections except Array are immutable, and they have a MutableMap, MutableSet, MutableQueue, etc

pbiggar commented 2 years ago

All that conversation aside, I think we want to merge this as is, right?

Just reran the CI to see if that'll get the build green.

Lomand commented 2 years ago

Well, I guess it can be considered done now.

Some notable typos:

You {e cannot } add an [int] and a [float]

turned into 

You add [int] and a [float]

It Looks like it was that way literally for years. Now fixed.

pbiggar commented 2 years ago

Wow, this is phenomenal! Thank you so much.