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

python: Documentation fixes for dazl.ledger.api_types. #179

Merged
merged 2 commits into from
Mar 16, 2021

Conversation

da-tanabe
Copy link
Contributor

@da-tanabe da-tanabe commented Mar 16, 2021

Some minor documentation fixes here and there, mostly in dazl.ledger.api_types.

+-----------------------------------+--------------------------------------------------------------+
| Commands |
+-----------------------------------+--------------------------------------------------------------+
| :class:`Command` | abstract base class of all commands |
Copy link
Collaborator

Choose a reason for hiding this comment

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

any reason the descriptions here are not capitalized and lack punctuation

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 copied the style of the table at the top of https://docs.python.org/3/library/collections.html ; I'm not a huge fan of this treatment, but when in Rome I guess…

python/dazl/ledger/__init__.py Outdated Show resolved Hide resolved
python/dazl/ledger/__init__.py Outdated Show resolved Hide resolved
| :class:`ArchiveEvent` | event raised when a contract is archived on the transaction |
| | stream |
+-----------------------------------+--------------------------------------------------------------+
| :class:`ExerciseResponse` | the response from an exercise, including the choice's return |
Copy link
Collaborator

Choose a reason for hiding this comment

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

"event raised when a contract choice is exercised, including...."

Copy link
Collaborator

Choose a reason for hiding this comment

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

This might also ought to be an ExerciseResponseEvent class, if that can be done in a backward compat way. (So write-side commands are Command and read-side events are Event)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the comment below; I'm (now) trying to avoid calling things Event's that are not on the Ledger API, and essentially would like to one day declare Event the same way it's declared in the TypeScript bindings (essentially CreateEvent | ArchiveEvent: https://docs.daml.com/app-dev/bindings-ts/daml-ledger/modules/_index_.html#event

Some names, like ExerciseResponse, are actually stolen from the TypeScript library as well. :-)

There has been some talk of ArchiveEvent's gaining the same set of data as a CreateEvent, and if this happens, I'd even more so like to give the Event suffix special treatment as "a thing on the ledger that represents a state change of a specific contract", as opposed to dazl's previous looser treatment of it as anything that happened on the read side.

Arguably the introduction of read streams already breaks the naming pattern of calling all read-side objects events (because a stream is a sequence of events), so from that perspective as well, I think tightening up the definitions of these suffices make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some clarifying text. As I was doing that, I reminded myself that ExerciseResponse contains events as well (just like a stream does); so even more so being more restrictive about what is and isn't an Event makes sense, I think:

class ExerciseResponse:
    result: Optional[Any]
    events: Sequence[Union[CreateEvent, ArchiveEvent]]

For historical reasons I'm not ready to pull the trigger on Event = Union[CreateEvent, ArchiveEvent] even though it should probably read that way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense to me.

.. autoclass:: ArchiveEvent
:members:

Other read-side types
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've always liked the distinction between 'read' and 'write' sides of the API, but the write side types might should be denoted as such.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

| :class:`ExerciseResponse` | the response from an exercise, including the choice's return |
| | value (if any) |
+-----------------------------------+--------------------------------------------------------------+
| :class:`Boundary` | indicates a point where an event stream can be subsequently |
Copy link
Collaborator

Choose a reason for hiding this comment

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

BoundaryEvent?

Copy link
Contributor Author

@da-tanabe da-tanabe Mar 16, 2021

Choose a reason for hiding this comment

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

Another thing I agonized over!

I went back and forth on this; I opted not to call it an Event in order to align with the more narrow definition of events over the Ledger API and in the TypeScript API (CreateEvent/ArchiveEvent); boundaries aren't really events as they are mere indications of places where streams could be resumed if you wanted to. They aren't necessarily repeatable because the HTTP JSON API doesn't necessarily promise it will give you the same boundaries if you requested a stream again later.

From an API standpoint you won't see them much, as I expect most clients will only be concerned with Ledger API events and not necessarily offset resuming, especially in the absence of a backing store where such offset could information could be stored:

# creates only (sweep)
conn = dazl.connect(...)
async with conn.stream("Some:Template") as stream:
    async for cev in stream.creates():
        print(f"{cev.contract_id} -> {cev.payload}")
# creates and archives--in other words, all "events" as you would see them documented elsewhere
conn = dazl.connect(...)
async with conn.stream("Some:Template") as stream:
    async for ev in stream.events():
        if isinstance(ev, CreateEvent):
             print(f"CREATE {ev.contract_id} -> {ev.payload}")
        elif isinstance(ev, ArchiveEvent):
             print(f"ARCHIVE {ev.contract_id} -> {ev.payload}")
        else:
             # no other type of Ledger API event
# EVERYTHING, including things that aren't "events"
conn = dazl.connect(...)
async with conn.stream("Some:Template") as stream:
    async for i in stream.items():
        if isinstance(i, CreateEvent):
             print(f"CREATE {i.contract_id} -> {i.payload}")
        elif isinstance(i, ArchiveEvent):
             print(f"ARCHIVE {i.contract_id} -> {i.payload}")
        elif isinstance(i, Boundary):
             print("BOUNDARY offset {i.offset}")
        else:
             # no other kind of object in the stream

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like that. (Not totally convinced on the use of stream methods as a filter specifier, but the whole thing is generally such a big improvement regardless.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep that was a tough design decision too; it mostly ended up this way because the server will send all this data down anyway (there's no way to ask the ledger for "just creates"), so the filter is client-side. I prototyped a version where stream had a bunch of parameters but I ended up just having to pass all that state down into the created stream anyway, not to mention trying to figure out how to make mypy happy.

At least with the functions on the stream, the typing rules are straightforward; otherwise I'd need a type parameter on the stream—and this is precisely where I punched out of trying to add them as parameters to the stream call, because mypy isn't nearly powerful enough to express this. :-/

@da-tanabe da-tanabe force-pushed the python-ledger-docs-fixes branch from 8673291 to 98d6604 Compare March 16, 2021 16:38
@da-tanabe da-tanabe merged commit 348e56e into master Mar 16, 2021
@da-tanabe da-tanabe deleted the python-ledger-docs-fixes branch March 16, 2021 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants