Open gaku-sei opened 4 years ago
~The {withIndexes}
option is not yet implemented, but the rest seems to be working as expected.~
Oh this is very good, thanks a lot. I will review with care soon.
Thank you very much @fakenickels. Notice that it's very wip and some things should probably change (like the way the array is updated in the setter).
While writing the description I also wondered if having the constructor with index alongside the "global" one wouldn't be better (it would also make the withIndexes option useless):
type _ field =
| Hobbies : string array field (* like usual *)
| HobbiesAt : int -> string option field
We could even have an unsafe *AtExn
for when the Field component is used in a map
loop (which is an extremely common case):
type _ field =
| Hobbies : string array field (* like usual *)
| HobbiesAt : int -> string option field
| HobbiesAtExn : int -> string field
Small update: I implemented the above, and dropped the ppx_tools
from the esy file for now (probably better to add it in a different PR).
It should be more or less ready for review, we just need to solve two issues:
1 - The generated field
type could take two args instead of one. So that the first one would represent the getter, and the second one the setter
2 - How should we update the array in the setter? I'm using the xs.(index) <- value
syntax for now since it's the fastest, but it's not immutable anymore.
Sorry about taking so long to give feedback, but I promise I'll be taking a look til the end of this week
Sorry for the late reply, and thank you very much for your review @fakenickels
When implementing this solution I was also considering some "refactor" but didn't want to change too many things. Also because I don't know what the plans are to make the code base scales.
Do you have any ideas on how this could be refactored? Should I just split the code into functions, or is it ok to use the [%expr ...]
annotation to make the code a bit less verbose?
Hey there @gaku-sei !
I agree 100% with you, the code is really verbose currently and we could make more usage of Ast helpers from OCaml. About using [%expr]
, i'm in for it!
Just to let you know I had to change a little bit the code to make it compatible with the new BS
@gaku-sei sorry about taking so long to get updates here. I'm willing to merge this already, but could we split the code in more functions or use Ast_helper or ppx_metaquot to make it less verbose like you suggested?
Target
The idea of this pr is to generate gadt constructors with an index attribute when the field is of type array.
This:
Will generate:
With the above example we can now type:
This becomes extremely useful once used with the new
Field
component introduced here. Since one can now write:Within a loop it can be:
Js.Dict
We could also consider adding this feature for
Js.Dict.t
with a key being astring
. Most of the work is already done/prepared.One word on the getter
Currently, the getter is a bit awkward, since you have to pass an optional value in order to update the field (with
*At
, not with*AtExn
).In order to allow the new provided value to be a
string
here instead of anoption(string)
, we have to make the generatedfield
type change, and accept two arguments:When setting an array's value using
*At
and passingNone
, nothing happens: