Skip to content

Conversation

@jamesls
Copy link
Member

@jamesls jamesls commented Aug 25, 2015

This PR updates the shorthand documenter to work with the latest changes to the shorthand code (#1461). Specifically:

  • It does not use a hardcoded list of "shapes" like the previous approach did.
  • It documents the newly supported syntax (nested dictionaries)

It also changes some small things in the way docs are generated:

  • The --argname is removed in the shorthand example. This is to make the shorthand syntax consistent with the JSON example syntax.
  • The type names are added instead of generic values. Before we'd have Key=value,Foo=value. Now we'll have Key=string,Foo=float.
  • The single sentence description has been removed. It's not possible to automatically generate these.

And finally, there's a few areas of improvement I think we'll need to make in the future:

  • Right now, shorthand examples are only generated for code of a stack depth of 3 or less. This is because sufficiently complicated structures are not readable in shorthand syntax. We'd need to come up with a way to break these across multiple lines.
  • Recursive shapes are documented as (... recursive ...), which is how JSON params are documented. This might be an area for improvement.

cc @kyleknap @mtdowling @rayluo

Copy link
Contributor

Choose a reason for hiding this comment

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

Is SHORTHAND_SHAPES still used?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, forgot to remove. Will update this.

@kyleknap
Copy link
Contributor

It looks fine. Had a few comments, but I think they would have been caught with pr-check. Did you run it? It would be good to get the tests to pass as well. Probably will need to use OrderedDict

@jamesls
Copy link
Member Author

jamesls commented Aug 27, 2015

Feedback should be incorporated. Build is passing.

Looks like there's one test case I missed, I need to add a doc for the changed list type, <value1> .... I'll get a test added for that.

Was able to remove a condition that no longer applied.
@jamesls
Copy link
Member Author

jamesls commented Aug 27, 2015

@kyleknap Ok, I believe everything's been incorporated, including updated tests. Should be good to look at again.

@jamesls jamesls added the pr:needs-review This PR needs a review from a Member. label Aug 27, 2015
@kyleknap
Copy link
Contributor

Looks good. 🚢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr:needs-review This PR needs a review from a Member.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants