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

Fix errors in Python decorator API documentation #2124

Merged
merged 6 commits into from
Apr 3, 2018
Merged

Conversation

SeanTAllen
Copy link
Contributor

It doesn't exist and using it causes a syntax error.

[skip ci]

It doesn't exist and using it causes a syntax error.

[skip ci]
@SeanTAllen SeanTAllen requested a review from JONBRWN March 22, 2018 16:42
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]
@@ -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")`.
Copy link
Contributor

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

Copy link
Contributor

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?

Copy link
Contributor

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"?

Copy link
Contributor

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")

@@ -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")
Copy link
Contributor

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)

@@ -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`.
Copy link
Contributor

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)

@@ -187,15 +187,15 @@ Partition Keys must correctly support the `__eq__` (`==`) and `__hash__` operato

### PartitionFunction
Copy link
Contributor

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


##### `encode(data)`
##### `encoder(data)`
Copy link
Contributor

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

Copy link
Contributor

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?

Copy link
Contributor

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?

@@ -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):
Copy link
Contributor

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)

@SeanTAllen SeanTAllen changed the title Remove references to @state decorator from Python docs Fix errors in Python generator API documentation Mar 23, 2018
Removes usage of classes when objects can now be functions. Removes
usage of TCPSinkEncoder and TCPSourceDecoder which no longer exist.

[skip ci]
@JONBRWN
Copy link
Contributor

JONBRWN commented Mar 23, 2018

@nisanharamati made some updates, review it when you have a chance. Thanks!

@@ -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.
Copy link
Contributor

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.

@@ -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"):
Copy link
Contributor

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

@nisanharamati
Copy link
Contributor

@JONBRWN Please give this a pass?
If you don't like these changes, feel free to overrule them.

I did the following:

  1. Organize everything related to Sources, Sinks, and Computations in groups on the page.
  2. Within the sources and sinks groups, I further reorganized to use the following hierarchy: Source -> TCPSource | KafkaSource -> Source Decoder -> Example TCP | Example Kafka and similarly for sinks
  3. Within the Computations group, I organized it so that the decorators are subheaders of the Computation and StateComputation levels respectively, and each with their own example.

@nisanharamati
Copy link
Contributor

Still missing:
something about tcp_parse_input_addrs, tcp_parse_output_addrs, kafka_parse_source_options, and kafka_parse_sink_options

@aturley
Copy link
Contributor

aturley commented Mar 28, 2018

@SeanTAllen title says "Fix errors in Python generator API documentation", but I think you meant "Fix errors in Python decorator API documentation", correct?

@SeanTAllen
Copy link
Contributor Author

omg, i cant believe i keep typing generator rather than decorator.

@SeanTAllen SeanTAllen changed the title Fix errors in Python generator API documentation Fix errors in Python decorator API documentation Mar 28, 2018
@JONBRWN
Copy link
Contributor

JONBRWN commented Mar 28, 2018

@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?

@aturley
Copy link
Contributor

aturley commented Apr 3, 2018

This looks good for now.

@aturley aturley merged commit 919c00c into master Apr 3, 2018
wallaroolabs-wally added a commit that referenced this pull request Apr 3, 2018
@nisanharamati nisanharamati deleted the no-state-decorator branch April 3, 2018 02:20
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants