Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 70 additions & 36 deletions compiler/graindoc/docblock.re
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,72 @@ let for_value_description =

let (args, return_type) = types_for_function(~ident, vd);

// extract the documented parameters and put into a tuple for easy lookup
let param_attributes: list((string, Comment_attributes.t)) =
List.filter_map(
attr =>
switch (attr) {
| Comment_attributes.Param({attr_id}) =>
switch (attr_id) {
| PositionalParam(idx) => Some((string_of_int(idx), attr))
| LabeledParam(name) => Some((name, attr))
}
| _ => None
},
attributes,
);

let match_label_to_arg = arg => {
List.find_opt(((name, _)) => name == arg, param_attributes);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};

let documented_args =
switch (param_attributes) {
| [] => [] // no documented params so don't add to the table
| _ =>
switch (args) {
| None => []
| Some(function_args) =>
List.mapi(
(index, (lab, exp)) => {
let param_id =
switch (lab) {
| Typedtree.Labeled(l)
| Default(l) => l.txt
| _ => string_of_int(index)
};

// First try to match by name, then by position
let matched =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

switch (match_label_to_arg(param_id)) {
| Some(_) as result => result
| None => match_label_to_arg(string_of_int(index))
};

// Extract documentation from the matched attribute
let documentation =
switch (matched) {
| Some((_, Comment_attributes.Param({attr_desc}))) => attr_desc
| _ => ""
};

// Get parameter type
let param_type =
switch (lookup_arg_by_label(param_id, args)) {
| Some((Labeled(_), typ)) => Printtyp.string_of_type_sch(typ)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: We could use an or pattern here to reduce the number of branches.

| Some((Default(_), {desc: TTyConstr(_, [typ], _)})) =>
Printtyp.string_of_type_sch(typ)
| Some((_, typ)) => Printtyp.string_of_type_sch(typ)
| None => ""
};

{param_id, param_type, param_msg: documentation};
},
function_args,
)
}
};

let (deprecations, since, history, params, returns, throws, examples) =
List.fold_left(
(
Expand Down Expand Up @@ -414,41 +480,9 @@ let for_value_description =
throws,
examples,
)
| Param({attr_id: param_id, attr_desc: param_msg}) =>
let (param_id, param_type) =
switch (param_id) {
| PositionalParam(idx) =>
switch (lookup_type_expr(~idx, args)) {
| Some((_, typ)) => (
string_of_int(idx),
Printtyp.string_of_type_sch(typ),
)
| None => raise(MissingUnlabeledParamType({idx: idx}))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}
| LabeledParam(name) =>
switch (lookup_arg_by_label(name, args)) {
| Some((Labeled(_), typ)) => (
name,
Printtyp.string_of_type_sch(typ),
)
// Default parameters have the type Option<a>; extract the type from the Option
| Some((Default(_), {desc: TTyConstr(_, [typ], _)})) => (
"?" ++ name,
Printtyp.string_of_type_sch(typ),
)
| _ => raise(MissingLabeledParamType({name: name}))
}
};

(
deprecations,
since,
history,
[{param_id, param_type, param_msg}, ...params],
returns,
throws,
examples,
);
| Param(_) =>
// do nothing as we precomputed them
(deprecations, since, history, params, returns, throws, examples)
| Returns({attr_desc: returns_msg}) =>
switch (returns) {
| Some(_) =>
Expand Down Expand Up @@ -502,7 +536,7 @@ let for_value_description =
deprecations: List.rev(deprecations),
since,
history: List.rev(history),
params: List.rev(params),
params: documented_args,
returns,
throws: List.rev(throws),
examples: List.rev(examples),
Expand Down
63 changes: 63 additions & 0 deletions compiler/test/graindoc/params.expected.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
---
title: Params
---

## Values

Functions and constants included in the Params module.

### Params.**test1**

```grain
test1: (a: Number, b: Number) => Number
```

Parameters:

|param|type|description|
|-----|----|-----------|
|`a`|`Number`|This is the first|
|`b`|`Number`|This is the second|

### Params.**test2**

```grain
test2: (a: Number, b: Number) => Number
```

Parameters:

|param|type|description|
|-----|----|-----------|
|`a`|`Number`||
|`b`|`Number`|This is the third|

### Params.**test3**

```grain
test3: (a: Number, b: Number, c: Number) => Number
```

Parameters:

|param|type|description|
|-----|----|-----------|
|`a`|`Number`|This is the first|
|`b`|`Number`|This is the third|
|`c`|`Number`||

### Params.**test4**

```grain
test4: (a: Number, b: Number, c: Number, a) => Number
```

Parameters:

|param|type|description|
|-----|----|-----------|
|`a`|`Number`|This is the a|
|`b`|`Number`|This is the b|
|`c`|`Number`||
|`3`|``|This is the 3|

25 changes: 25 additions & 0 deletions compiler/test/graindoc/params.input.gr
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
module Params

/**
* @param a: This is the first
* @param b: This is the second
*/
provide let test1 = (a, b) => a + b

/**
* @param b: This is the third
*/
provide let test2 = (a, b) => a + b

/**
* @param b: This is the third
* @param a: This is the first
*/
provide let test3 = (a, b, c) => a + b + c

/**
* @param a: This is the a
* @param b: This is the b
* @param 3: This is the 3
*/
provide let test4 = (a, b, c, _) => a + b + c
1 change: 1 addition & 0 deletions compiler/test/suites/graindoc.re
Original file line number Diff line number Diff line change
Expand Up @@ -37,4 +37,5 @@ describe("graindoc", ({test, testSkip}) => {
"Attribute @returns is only allowed to appear once.",
[|"--current-version=v0.2.0"|],
);
assertGrianDocOutput("params", "params", [|"--current-version=v0.2.0"|]);
});
Loading