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

Jepsen/mdbook #2297

Merged
merged 7 commits into from Apr 25, 2023
Merged

Jepsen/mdbook #2297

merged 7 commits into from Apr 25, 2023

Conversation

0xJepsen
Copy link
Contributor

I added some more to the documentation here

  • Events
  • Subscriptions

contributes to #2282

Copy link
Collaborator

@prestwich prestwich left a comment

Choose a reason for hiding this comment

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

High-level changes:

It would be good to route users to the abigen!-based interfaces for accessing these tools, as they're simpler and higher level. This documentation is a good explanation of how abigen works under the hood

We should also mention the difference between Event::stream and Event::subscribe. the stream interface is polling, and will work with any provider, but have increased cost and latency. Subscribe is based on WS/IPC notifications, and is limited to those providers

book/events/events.md Outdated Show resolved Hide resolved
book/events/events.md Outdated Show resolved Hide resolved
book/events/events.md Outdated Show resolved Hide resolved
book/subscriptions/subscriptions.md Show resolved Hide resolved
@0xJepsen
Copy link
Contributor Author

Thanks for the feedback, I went and made some of the easy changes. I have yet to distinguish the difference between Event::stream and Event::subscribe like you recommended but will look to getting around to it soon.

@prestwich
Copy link
Collaborator

Thanks for the feedback, I went and made some of the easy changes. I have yet to distinguish the difference between Event::stream and Event::subscribe like you recommended but will look to getting around to it soon.

did you intend to do this as part of this PR? or in a future one?

Copy link
Collaborator

@prestwich prestwich left a comment

Choose a reason for hiding this comment

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

Since this is at risk of getting stale, I'm going to re-run CI and merge if succesful. Followup work can be a separate PR

@prestwich
Copy link
Collaborator

CI failures unrelated

@prestwich prestwich merged commit 0b2bf4f into gakonst:master Apr 25, 2023
14 of 18 checks passed
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.

None yet

2 participants