fix: bring documentation and tutorials up to date with latest tooling #106
Conversation
There was a problem hiding this comment.
This is great - it is so good to finally see the tooling starting to smooth out its rough edges. And I love how you've made the flow through the different tutorials more consistent.
My comments are mostly nits except for the one on the printout of the generated Python bindings. I bounced off that pretty hard I'm afraid. But the rest is great.
| See [`componentize-py`'s examples](https://github.com/bytecodealliance/componentize-py/tree/main/examples) to try out build HTTP, CLI, and TCP components from Python applications. | ||
|
|
||
|
|
||
| ### Building a Component that exports an interface |
There was a problem hiding this comment.
Nit: capitalisation in this title is a bit random (as, admittedly, it was in the previous iteration).
(Longer term we should pick either Sentence case or Title Case and align all our headings around it. But that's definitely out of scope for this PR!)
There was a problem hiding this comment.
Yeah maybe we add a style guide
| # app.py | ||
| import example | ||
|
|
||
| class Add(example.Example): |
There was a problem hiding this comment.
The explanatory paragraph said "we now implement a class for the add interface rarher than the example world, but this still inherits from example.Example. It would be good to clarify what makes this implement that specific interface (of all the ones in the example world). Is it the class name?
There was a problem hiding this comment.
Yes class name changes
| # Get an add component that does not import the WASI CLI world | ||
| $ wget https://github.com/bytecodealliance/component-docs/raw/main/component-model/examples/example-host/add.wasm | ||
| # Validate the component against the checksum | ||
| $ echo "7a64aa30ac7d91b03448001dcdc23760a80c58ecaf220bc70998d49b88820440 add.wasm" | sha256sum --check |
There was a problem hiding this comment.
This feels like it could easy slip out of sync if we ever rebuild add.wasm.
There was a problem hiding this comment.
Should we remove the checksum since it is a module rather than an executable binary? Or we can add a add-checksum file next to it to make it more obvious
There was a problem hiding this comment.
I'd probably remove the checksum because I have a childlike faith in the Wasm sandbox. Having a checksum file available for people who want it is probably a decent plan, as long as we can automate maintenance. Is the .wasm file built in CI or do authors build locally and check it in?
There was a problem hiding this comment.
authors build locally and check it in
this one. Can can have CI for it in the future.
There was a problem hiding this comment.
I removed the checksum
| assert(isinstance(lift_callee0, wasmtime.Func)) | ||
| self.lift_callee0 = lift_callee0 | ||
| def add(self, caller: wasmtime.Store, x: int, y: int) -> int: | ||
| ret = self.lift_callee0(caller, _clamp(x, -2147483648, 2147483647), _clamp(y, -2147483648, 2147483647)) |
There was a problem hiding this comment.
Yikes! Does the user need to understand all this stuff?! Could we elide it, or move it to an "internals" section? If not, can we clarify to readers what they should be looking at?
There was a problem hiding this comment.
This is all generated code that i wanted to show them in the docs. Normally in the docs, we have not shown the bindings that are generated -- so I will remove this section
kate-goldenring
force-pushed
the
update-language-guides
branch
from
afabf3b to
987e170
Compare
Signed-off-by: Kate Goldenring <kate.goldenring@fermyon.com>
kate-goldenring
force-pushed
the
update-language-guides
branch
from
987e170 to
a4fbcbd
Compare
There was a problem hiding this comment.
LGTM but I would like get confirmation that https://github.com/bytecodealliance/component-docs/pull/106/files#r1527609090 is a non-issue please. Otherwise all good! Thanks!
A lot of the language guides were out of date. This updates:
wasmtime/implementations) when they were moved toruntimescomponentize-pyandwasmtime-pycompatibility to avoid this issue Python tooling docs fail with NotImplmentedError #84I ran through everything and got it working. Ideally we can get this in and iterate if there is a lot of syntactical feedback