Python doc polish #1757
Python doc polish #1757
Conversation
Previously, it was a TypedDict, but that was causing problems. Sphinx seems to be broken, such that none of the fields of the class were being rendered into the doc. Thinking on it further, I decided that a NamedTuple makes more sense here anyways, since tuples are immutable and this output value isn't something someone should ever build or modify. And, NamedTuple is getting its fields properly rendered by Sphinx.
Note that none of the changes to .py files impact functionality in any way, because the changes are restricted to "docstrings", which to the Python interpreter are simply no-op statements. However, one caveat to that is that much of these docstring changes DO affect the functionality of automated test runs, because all of the code examples (blocks beginning with `>>> `) are "doctests", which are exercised via the test framework. The index.rst files are the top-level templates for generating the documentation, and the "automodule"/"autoclass"/etc statements pull in the docstrings from the source code.
|
Eugene! this documentation is really amazing! thanks for putting this together. I took a look at all the rendered documentation and I want to share my comments. Most of these comments are broad across all the documentation:
Are we sure the
|
|
@PirosB3 thanks for the great feedback!
Yeah, I noticed that myself and wasn't sure whether it was worth addressing. This bulk of the SRA client was generated from the SRA spec (see
Good call, I'll add |
The generated code was transforming the order structure, from the camel case field name format in the spec, into the snake case field name format expected by Python convention. With this problem in place, the only way to take an order from a relayer and send it to a contract (for fill, cancel, etc) was to manually transform the field names, one by one, into a new structure.
Then convert it to JSON before sending it to the relayer.
|
I spent my Future Focused Friday knocking off that "Future Work" item in my PR description about ease of JSON conversion. The SRA demo now goes from native Python, converted to JSON for submission to relayer, and then back from relayer JSON to native Python, demonstrating both a fill and a cancel. |
And have taker take it.
Doing that exposed excessive use of the verbose NETWORK_TO_ADDRESSES[NetworkId.Ganache] construct, so simplified that, which ripped into eliminating other temporary variables that had been used to hold specific contract addresses. Also cleaned up some misplaced import statements.
|
Hi @feuGeneA thanks for the great update! Have you re-rendered documentation since you made those changes we discussed? can I go ahead with another review? |
|
@PirosB3 yes, I did update the rendered docs at the links in my initial comment with the latest changes. Please do go ahead with another review :) Thanks! |
Description
After the recent bounty to fully document all of our Python offerings with code examples, I polished things up, mostly copy editing (incl. code examples) for conciseness and improved flow.
The rendered docs are accessible at the links below. I suggest not reviewing the "Improve documentation" commit (which, btw, encompasses the bulk of the changes in this PR), and instead just review the rendered docs.
Do note that virtually all of this documentation is rendered from "docstings" in the source code. (In Python, a docstring is simply a string literal that's ignored at runtime but that documents a module, method, etc, depending on its context.) The rendering is done by Sphinx, which to my understanding is powerfully customizable, so in theory we could make the rendering look however we wanted. However, I don't know much about this tool, as I have barely scratched the surface of its usage.
Also note that all of the code examples are "doctests." (In Python, doctests are code snippets that can be exercised in an automated way.) When we run our unit tests (in CI and locally), we also test our doctests. So, when CI is clean, you know those examples are working.
Types of changes
Future work
Outside the scope of this PR...
Ensuring ease of conversion between Python object and JSONEdit: I knocked this out on Future Focused Friday.
In scrubbing this code with such scrutiny, it has come to my attention that there's one remaining thing that's still not as easy as it should be: moving order objects between Web3.py-compatibility and Relayer-compatibility.To submit an order to our contracts via web3.py the order object must have Python-native data types (asset data asbytes, numbers asints, etc), but when working with a Relayer the order must be in JSON (asset data as hex strings, numbers as strings, etc). We have provided convenience methods for converting between the two types, but we should write some more tests/examples/documentation around that.Specifically, I think we should modify the SRA Client examples to declare the order as a web3.py-compatible object (currently it starts out as JSON), then convert it to JSON and submit it to the relayer (launch kit), then have a separate taker account get the order and fill it (which will require converting it back from JSON to a web3.py-compatible object). Doing this may well reveal worthwhile changes to our Python API.Integrating docs with 0x.org
The URLs for these docs are ugly. You see those staging URLs above, but they're not really any different from the "real" doc locations. (Just take out the
-stagingpart, and you have it.) We should consider altering the rendering to fit our design motifs (see discussion of Sphinx above), and we should host them at friendlier URLs. If desirable, we may be able to get Sphinx to render everything into one giant doc, rather than one per package.Checklist
pip installinstructions are consistent across the docs (some were missing it).*_with_http_info()methods in SRA client.