-
Notifications
You must be signed in to change notification settings - Fork 68
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
Fix errors in Python decorator API documentation #2124
Conversation
It doesn't exist and using it causes a syntax error. [skip ci]
The `Wallaroo API` section of the Python API docs was not updated to use the latest API and referenced old and unused decorators. This change updates the file to remove unused decorators and update the naming/formatting of the ones which still exist. [skip ci]
book/python/api.md
Outdated
@@ -229,21 +229,21 @@ The TCP Source Decoder is responsible for two tasks: | |||
1. Telling Wallaroo _how many bytes to read_ from its input connection. | |||
2. Converting those bytes into an object that the rest of the application can process. | |||
|
|||
Your function must be decorated with `@decoder(header_length, fmt)`. | |||
Your function must be decorated with `@wallaroo.decoder(header_length=header_length, length_fmt="length_fmt")`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
length_fmt
doesn't take a name string here. It takes an argument that is passed to struct.unpack
following https://docs.python.org/2/library/struct.html#format-strings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's described in the ####
length_fmt` section below, but the example probably shouldn't use an invalid argument either?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any thoughts on what you'd propose as an optimal replacement to "length_fmt"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm tempted to use real values from our examples, like @wallaroo.decoder(header_length=4, length_fmt=">I")
book/python/api.md
Outdated
@@ -254,8 +254,8 @@ Return a python a python object of the type the next step in the pipeline expect | |||
A complete TCPSourceDecoder example that decodes messages with a 32-bit unsigned integer _payload\_length_ and a character followed by a 32-bit unsigned int in its _payload_: | |||
|
|||
```python | |||
@decoder(header_length=4, length_fmt=">L") | |||
def decode(self, bs): | |||
@wallaroo.decoder(header_length=4, length_fmt=">L") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've been using ">I"
in our examples, so this should probably match (they're functionally identical)
book/python/api.md
Outdated
@@ -187,15 +187,15 @@ Partition Keys must correctly support the `__eq__` (`==`) and `__hash__` operato | |||
|
|||
### PartitionFunction | |||
|
|||
A partition function class must be wrapped in the `@partition_function` decorator and return the appropriate [Key](#key) for `data`. | |||
A partition function class must be wrapped in the `@wallaroo.partition` decorator and return the appropriate [Key](#key) for `data`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a class anymore!
should be
A partition function must be decorated with ...
(also note decorated with
instead of wrapped in
)
book/python/api.md
Outdated
@@ -187,15 +187,15 @@ Partition Keys must correctly support the `__eq__` (`==`) and `__hash__` operato | |||
|
|||
### PartitionFunction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Title should probably just be Partition
book/python/api.md
Outdated
|
||
##### `encode(data)` | ||
##### `encoder(data)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should stay as encode
as it's a function, not a class. It should be named after the action it performs, not the object performing it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I think about it, this section is weird. It looks like it's left over from when encode
was a field of the Encoder
class, but isn't anymore. Maybe the entire section should be revised to reflect that it's just a decorated function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all of our examples use encoder
, should we update all of those as well?
book/python/api.md
Outdated
@@ -218,7 +218,7 @@ A complete `TCPSinkEncoder` example that takes a list of integers and encodes it | |||
|
|||
```python | |||
@encoder | |||
def encode(self, data): | |||
def encoder(data): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto here: should be def encode(data)
Removes usage of classes when objects can now be functions. Removes usage of TCPSinkEncoder and TCPSourceDecoder which no longer exist. [skip ci]
@nisanharamati made some updates, review it when you have a chance. Thanks! |
book/python/api.md
Outdated
@@ -141,7 +140,7 @@ Return the complete list of topology tuples. This is the topology structure Wall | |||
|
|||
A stateless computation is a simple function that takes input, returns an output, and does not modify any variables outside of its scope. A stateless computation has _no side effects_. | |||
|
|||
A `computation` class must be wrapped by the `@computation(name)` decorator, which takes the name of the computation as an argument. | |||
A `computation` function must be decorated with the `@wallaroo.computation(name="computation name")` decorator, which takes the name of the computation as an argument. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should also include @wallaroo.computation_multi
as an option.
book/python/api.md
Outdated
@@ -167,7 +166,7 @@ def compute(data): | |||
A Computation that returns both its input integer and double that value. If the incoming data isn't an integer, we filter (drop) the message by returning `None`. | |||
|
|||
```python | |||
@computation_multi("doubledouble"): | |||
@wallaroo.computation_multi(name="doubledouble"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No colon at the end of a decorator
@JONBRWN Please give this a pass? I did the following:
|
Still missing: |
@SeanTAllen title says "Fix errors in Python generator API documentation", but I think you meant "Fix errors in Python decorator API documentation", correct? |
omg, i cant believe i keep typing generator rather than decorator. |
@nisanharamati this looks good to me but I don’t have as much of an opinion on documentation as others. @aturley any thoughts on the changes? |
This looks good for now. |
It doesn't exist and using it causes a syntax error.
[skip ci]