Closed Burnleydev1 closed 1 year ago
The map_value
and map_value'
look good!
The second bullet point is almost good: bool_of_string
raises Invalid_argument
when the given string is not "true"
or "false"
, and that is supposed to happen everytime the pattern fails to match.
You can take inspiration from the some
function for instance, for what happens when a pattern fails to match.
You don't need to "implement" Lident
: it is already implemented in ast_pattern_generated.ml
. You just need to combine pexp_construct
, lident
, the value you've called ebool
in this PR, and none
, to create the ebool
we want.
Hello @panglesd, what if I use the bool_of_string_opt
since it has the option
which is what I think is what is added by the some
function, will it do the job, because bool_of_string_opt
says that it does this ( Convert the given string to a boolean. Return None if the string is not "true" or "false"
)
But it just hit me that ebool
is supposed to be (bool, 'a, 'b) t -> (expression, 'c, 'd) t
and not (bool option, 'a, 'b) t -> (expression, 'c, 'd) t
You could have used bool_of_string_opt
and matched on the result to know if it has worked or not. However, this function is only available since OCaml 4.05, and ppxlib supports OCaml starting from 4.04!
However, those functions are easy to reimplement.
Hello @panglesd,please I looked at some
and came did something like this
let bool' (T f0) =
T
(fun ctx loc x k ->
match x with
| Some x0 ->
ctx.matched <- ctx.matched + 1;
let k = f0 ctx loc (bool_of_string x0) k in
k
| _ -> fail loc "true")
which has the signature (bool, 'a, 'b) t -> (label option, 'a, 'b) t
Please I wish to ask if I am in the right direction.
I'm sorry for asking too many questions.
You don't need to take so much of the some
function: just look at the branch where the given value is not a Some _
.
Hello @panglesd, please i am having issues identifying the branch where the given value is not a Some _
please is it the | _ -> fail loc "Some")
?
please is there documentation on this, please I would like to read it and understand some
and other functions.
let some (T f0) = (* some takes as input a pattern [T f0] (supposed to match the inside of the [Some _]*)
T
(fun ctx loc x k -> (* it returns a pattern which: *)
match x with (* match whether [x] is [Some _] or [None]: *)
| Some x0 -> (* - in the [Some _] case it *)
ctx.matched <- ctx.matched + 1;
let k = f0 ctx loc x0 k in (* applies the pattern given as input to the value enclosed in the [Some] *)
k
| _ -> fail loc "Some" (* - otherwise, it fails with location [loc], saying that it expects a [Some] (see the definition of [fail] *))
Hello @panglesd, thanks for explaining how some
functions, I tried using the method where we explicitly contain what happens when its "true" and "false", describing what should do when fails. The string I passed in fail
is not correct yet but I just wanted you to checkout this new definition of bool'
Yes, that's perfect! You now have a function turning a (bool, 'c, 'd) Ast_pattern.t
into a (string, c, d) Ast_pattern.t
, the task 2. in your first post. (note that you did not use the map function, but that's not a problem of course)
Hello @panglesd, I am trying to carry out step 3 but I'm facing some difficulties, I took inspiration from this function below since it is the only place in Ast_pattern
when pexp_construct
and Lident
are used
let rec parse_elist (e : Parsetree.expression) acc =
Common.assert_no_attributes e.pexp_attributes;
match e.pexp_desc with
| Pexp_construct ({ txt = Lident "[]"; _ }, None) -> List.rev acc
| Pexp_construct ({ txt = Lident "::"; _ }, Some arg) -> (
Common.assert_no_attributes arg.pexp_attributes;
match arg.pexp_desc with
| Pexp_tuple [ hd; tl ] -> parse_elist tl (hd :: acc)
| _ -> fail arg.pexp_loc "list")
| _ -> fail e.pexp_loc "list"
and started with something like this
let ebool = Pexp_construct ({txt = Lident "true"; _} ,None)
but I am getting errors like Some record fields are undefined: loc
so I changed it to
let ebool = Pexp_construct ({txt = Lident "true"; loc = Location.none} ,None)
which gives no errors
but then ebool
here has just the expression_desc
signature instead of (bool, 'a, 'b) t -> (expression, 'c, 'd) t
that is expected
I am lost on how to proceed here.
I tried using the method from this documentation as well to get something like this
let ebool (parse_res : (bool, 'a, 'b) t) : (expression, 'a, 'b) t =
Pexp_construct ({txt = Lident bool'; loc = Location.none} ,None)
but it did not work either.
as I got the errors Error (warning 27): unused variable parse_res.
and
longident_loc * expression option -> expression_desc This variant expression is expected to have type (expression, 'a, 'b) t There is no constructor Pexp_construct within type t
Lident converts (label, 'a, 'b) t -> (longident, 'a, 'b) t
pexp_construct converts (longident, 'a, 'b) t -> (expression option, 'b, 'c) t -> (expression, 'a, 'c) t
and bool' converts (bool, 'a, 'b) t -> (label, 'a, 'b) t
ebool needs to convert (bool, 'a, 'b) t -> (expression, 'a, 'b) t
I am currently working on trying to link them
Hello @panglesd, I think I finally got it, I have pushed the changes here.
Very nice! Thank you @Burnleydev1!
Why did you decide to include bool'
in the .mli
file? and why naming it like this?
@panglesd, I noticed that other variables in the .ml had their signature in the .mli file and I named it like that because I noticed that each variable eg map has a map’ and int32 also has int32’.I don’t know why that’s the case but I just decide that bool should also have a bool ’
And I noticed that the map’ was the map but with location
I just saw how other vals were defined and used that as a template to define these ones but I can remove bool’ if it is not needed
Or change it to something more conventional
But the main reason is that bool
already existed and I did not know a better name, I removed it from .mli and it still built without any errors, so I will push a commit for the changes
@panglesd, could we name it bool_l
indicating that it converts bool to label?
If the existing helpers are called <type>'
, then I think it's sensible for this feature to continue that, rather than introduce a competing idiom. I agree the idiom is not great, but I think we should fix it wholesale in a separate PR rather than make it this PR's responsibility.
Thanks @panglesd, @ceastlund for the review.
fixes #399
map_value
function to turn a(bool, 'c, 'd) Ast_pattern.t
into a(string, c, d) Ast_pattern.t
pexp_construct
,lident
and the value previously created to defineebool
andpbool