Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Support autogenerated documentation for method results, in addition to parameters #161

Merged
merged 1 commit into from
Apr 7, 2021

Conversation

RyanGlScott
Copy link
Contributor

@RyanGlScott RyanGlScott commented Apr 7, 2021

Currently, the DescribedParams class allows for declaring the documentation for the parameters of methods, but not the results. This makes it insufficient for generating the sorts of documentation like what is written here in the handwritten SAW remote API docs.

This is an attempt at augmenting the DescribedParams class to allow documenting result values in addition to parameters. Some of the highlights are:

  • The class has now been renamed to DescribedMethod to reflect its more general purpose.
  • DescribedMethod now has two type parameters: params (which existed before) and result. There is a params -> result functional dependency since the params type is almost always a custom data type, whereas result can often be off-the-shelf data types like Value and ().
  • In addition to the parameterFieldDescription method, there is now a resultFieldDescription method. It has a default implementation of [] in the common case where result is () (i.e., nothing is returned).
  • There has been some reorganization of the autogenerated documentation so that there is a more clear separation of the parameters and results.

@RyanGlScott RyanGlScott requested a review from pnwamk April 7, 2021 13:11
Copy link
Contributor

@pnwamk pnwamk left a comment

Choose a reason for hiding this comment

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

This looks like a great improvement for all the servers!

One minor question -- in the generated docs, there is now for each method a Parameters section and a Return fields section, and I have this nagging feeling that they should match a little more. Would Parameter fields and Return fields be the most sensible similar "more similar" names? (I'm assuming the "fields" suffix is because we're literally talking about JSON object fields, in which case it seems reasonable to me to have it appear as such on both sections).

…o parameters

Currently, the `DescribedParams` class allows for declaring the documentation
for the _parameters_ of methods, but not the _results_. This makes it
insufficient for generating the sorts of documentation like what is written
[here](https://github.com/GaloisInc/saw-script/blob/7bb93c9b5c08422a49017e49772d8d2db3ff8fb0/saw-remote-api/docs/old-Saw.rst#running-proof-scripts)
in the handwritten SAW remote API docs.

This is an attempt at augmenting the `DescribedParams` class to allow
documenting result values in addition to parameters. Some of the highlights
are:

* The class has now been renamed to `DescribedMethod` to reflect its more
  general purpose.
* `DescribedMethod` now has two type parameters: `params` (which existed
  before) and `result`. There is a `params -> result` functional dependency
  since the `params` type is almost always a custom data type, whereas `result`
  can often be off-the-shelf data types like `Value` and `()`.
* In addition to the `parameterFieldDescription` method, there is now a
  `resultFieldDescription` method. It has a default implementation of `[]`
  in the common case where `result` is `()` (i.e., nothing is returned).
* There has been some reorganization of the autogenerated documentation so
  that there is a more clear separation of the parameters and results.
@RyanGlScott
Copy link
Contributor Author

RyanGlScott commented Apr 7, 2021

One minor question -- in the generated docs, there is now for each method a Parameters section and a Return fields section, and I have this nagging feeling that they should match a little more. Would Parameter fields and Return fields be the most sensible similar "more similar" names? (I'm assuming the "fields" suffix is because we're literally talking about JSON object fields, in which case it seems reasonable to me to have it appear as such on both sections).

Good question. I only chose the name "Return fields" since there was existing precedent for it in the SAW remote API's documentation (see here), so I simply copied it verbatim. But I agree with your desire for consistency, so I've added a commit which changes "Parameters" to "Parameter fields".

On a related note, I'm not entirely wedded to the way argo formats the autogenerated documentation. Currently it just uses subsections for the parameter/return fields, but it might look even nicer to use RST tables like in the SAW remote API's handwritten documentation. However, I don't think argo currently supports decoratively specifying tables like it does other forms of RST, so that would be a larger lift than what this PR implements. I'll open an issue about this point after this lands.

@pnwamk
Copy link
Contributor

pnwamk commented Apr 7, 2021

On a related note, I'm not entirely wedded to the way argo formats the autogenerated documentation. Currently it just uses subsections for the parameter/return fields, but it might look even nicer to use RST tables like in the SAW remote API's handwritten documentation. However, I don't think argo currently supports decoratively specifying tables like it does other forms of RST, so that would be a larger lift than what this PR implements. I'll open an issue about this point after this lands.

Yes, this is also a great point. The handwritten docs for saw-remote-api are easier to quickly parse and are much more pleasing to the eye than our current auto-formatted docs.

@RyanGlScott
Copy link
Contributor Author

I've opened #162 to track formatting this information into tables.

@RyanGlScott RyanGlScott merged commit b9b3edd into master Apr 7, 2021
@RyanGlScott RyanGlScott deleted the DescribedMethod branch April 7, 2021 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants