fix(graindoc): Add all function parameters to the table, even if missing documentation #2293
Conversation
marcusroberts
requested review from
phated,
ospencer and
alex-snezhko
as code owners
|
Take care when merging #2292 which makes changes to Graindoc. |
| ); | ||
|
|
||
| let match_label_to_arg = arg => { | ||
| List.find_opt(((name, _)) => name == arg, param_attributes); |
There was a problem hiding this comment.
I think List.assoc_opt or List.assq_opt might be what you want to use here https://ocaml.org/manual/5.3/api/List.html#1_Associationlists
compiler/graindoc/docblock.re
Outdated
| let matched = | ||
| switch (match_label_to_arg(param_id)) { | ||
| | Some(_) as result => result | ||
| | None => match_label_to_arg(string_of_int(index)) |
There was a problem hiding this comment.
Is the behaviour here that if you documented a param as x but the function only has position params we would map x to 0 for instance?
compiler/graindoc/docblock.re
Outdated
| let documentation = | ||
| switch (matched) { | ||
| | Some((_, Comment_attributes.Param({attr_desc}))) => attr_desc | ||
| | _ => "" |
There was a problem hiding this comment.
Should we be throwing some sort of exception here?
compiler/graindoc/docblock.re
Outdated
| | Some((Default(_), {desc: TTyConstr(_, [typ], _)})) => | ||
| Printtyp.string_of_type_sch(typ) | ||
| | Some((_, typ)) => Printtyp.string_of_type_sch(typ) | ||
| | None => "" |
There was a problem hiding this comment.
Should we be throwing some sort of exception here? And do you know if there is any real case this would occur?
compiler/graindoc/docblock.re
Outdated
| // Get parameter type | ||
| let param_type = | ||
| switch (lookup_arg_by_label(param_id, args)) { | ||
| | Some((Labeled(_), typ)) => Printtyp.string_of_type_sch(typ) |
There was a problem hiding this comment.
Nit: This could probably use an or pattern, instead of handling each branch with the same logic.
There was a problem hiding this comment.
Should we also test a function without the params documented at all, and an only positional argument.
We might also want to test the failing cases, i.e a param doesn't exist.
There was a problem hiding this comment.
You're right. Every other test covered this case as they all have undocumented params so I use those as my test case, but an explicit one would be better
| // Get parameter type | ||
| let param_type = | ||
| switch (lookup_arg_by_label(param_id, args)) { | ||
| | Some((Labeled(_), typ)) => Printtyp.string_of_type_sch(typ) |
There was a problem hiding this comment.
nit: We could use an or pattern here to reduce the number of branches.
| string_of_int(idx), | ||
| Printtyp.string_of_type_sch(typ), | ||
| ) | ||
| | None => raise(MissingUnlabeledParamType({idx: idx})) |
There was a problem hiding this comment.
Why do we never throw these errors anymore?
- Do we now allow labeling a positional arg with a label and the other way around? i.e:
/*
* @param x: test
*/
let test = (_ as x) => void
- What happens now if you document a non existing param, or too many positional params?
I see we removed it here and decided to handle the cases but are we sure we want to from a linting and correctness standpoint?
| }; | ||
|
|
||
| // First try to match by name, then by position | ||
| let matched = |
There was a problem hiding this comment.
Shouldn't the logic match specifially by name or position? i.e if we see an int its a position and a name its a position? This way we can continue to give the well formedness errors from before.
Partially Fixes #2281
Add all function parameters to the documentation table in the order they are defined.
Add an empty documentation field for any that are missing user supplied documentation in the comment above.
(New and Updated tests to follow)