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

Add metadata documentation #26

Merged
merged 9 commits into from
Aug 16, 2019
Merged

Conversation

rayjzeng
Copy link
Contributor

Summary

Adds documentation to the metadata interface for LibCST and cleans up some internal code.

Test Plan

Ran tox and pyre.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 14, 2019
docs/source/metadata.rst Outdated Show resolved Hide resolved
docs/source/usage.ipynb Outdated Show resolved Hide resolved
docs/source/visitors.rst Outdated Show resolved Hide resolved
@bgw
Copy link
Contributor

bgw commented Aug 14, 2019

Regarding Jimmy's comment about a usage notebook:

I think that'd be useful, but it's still nice to have a very small example on the same page as the reference itself.

I'd actually take the usage example (or an even more simplified example if that's possible) and move it to the top of the page, similar to what we do with the parser docs: https://libcst.readthedocs.io/en/latest/parser.html

I think notebooks should be reserved for more complex examples. E.g. it'd be great to have a metadata provider example in a notebook.

docs/source/metadata.rst Outdated Show resolved Hide resolved
docs/source/metadata.rst Outdated Show resolved Hide resolved
docs/source/metadata.rst Outdated Show resolved Hide resolved
libcst/_nodes/_internal.py Show resolved Hide resolved
@jimmylai
Copy link
Contributor

jimmylai commented Aug 14, 2019

Regarding Jimmy's comment about a usage notebook:

I think that'd be useful, but it's still nice to have a very small example on the same page as the reference itself.

I'd actually take the usage example (or an even more simplified example if that's possible) and move it to the top of the page, similar to what we do with the parser docs: https://libcst.readthedocs.io/en/latest/parser.html

I think notebooks should be reserved for more complex examples. E.g. it'd be great to have a metadata provider example in a notebook.

We can have the simple example in notebook and then include it from rst. That requires running a command to generate the rst from notebook and then we can include it. In that case, we'll have executable code and simple example. The other way to always make sure the example code is executable is to use doctest.
spatialaudio/nbsphinx#181 (comment)

@@ -0,0 +1,60 @@
.. _libcst-metadata:
Copy link
Contributor

Choose a reason for hiding this comment

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

This metadata page is pretty complex since it mixed the implementation detail with usage.
For user of metadata, they only need to know to use extends BatchableMetadataProvider, use get_metadata/set_metadata, etc.
User will be also interested about what's available metadata providers.
Can we explain more of those detail in the tutorial document?

All the MetadataProvider (BaseMetadataProvider/VisitorMetadataProvider/BatchableMetadataProvider) say they are low-level, which one we recommend user to use?

We introduced .resolve() and .resolve_many() but didn't provide examples. It's not clear how user can use them. Can we provide some example code?

The doc of Position Metadata will be more clearer if we link to the example at top. https://www.sphinx-doc.org/en/1.5/markup/inline.html#role-ref

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did some reorganization of the docs and added to the tutorial. I only added an example for resolve() and not one for resolve_many() as I think it should be pretty easy to extrapolate how to use resolve_many() from that point.


.. autoclass:: libcst.BaseMetadataProvider
.. autoclass:: libcst.VisitorMetadataProvider
.. autoclass:: libcst.BatchableMetadataProvider
Copy link
Contributor

Choose a reason for hiding this comment

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

The difference between VisitorMetadataProvider and BatchableMetadataProvider (the concept of batchable) is not clear from the docstring. Even though it links to BatchableCSTVisitor, we can probably say something here to make it more clear it's for performance sake.

Copy link
Contributor

@jimmylai jimmylai left a comment

Choose a reason for hiding this comment

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

LGTM

@rayjzeng rayjzeng merged commit 68699e2 into Instagram:master Aug 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants