Skip to content

docs: Update documentation #146

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

Merged
merged 17 commits into from
Sep 15, 2023
Merged

docs: Update documentation #146

merged 17 commits into from
Sep 15, 2023

Conversation

gismya
Copy link
Contributor

@gismya gismya commented Sep 15, 2023

  • I have added automatic tests where applicable
  • The PR title is suitable as a release note
  • The PR contains a description of what has been changed
  • The description contains manual test instructions

Changes

Starts chipping away at #100. Mostly moving the old RTD documentations to markdown and making sure it's referencing modern development workflow and current features in the client. An amalgamation of the old documentation, the python documentation, the work that @jimmycallin did in an older branch and some newly written things.

Test

Would be great if anyone could check the code examples for issues.

@gismya gismya requested a review from a team as a code owner September 15, 2023 00:06
Copy link
Contributor

@ffMathy ffMathy left a comment

Choose a reason for hiding this comment

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

This is amazing! Great work. Can't wait for this to be out so we can more easily contribute to the documentation too.

Minor comments.

README.md Outdated

### Error handling

A lot of the methods involve promises that might not always be resolved for various reasons. The examples will not have any error handling to keep them short, but make sure you have proper error handling when using the `EventHub`.
Copy link
Contributor

Choose a reason for hiding this comment

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

For the promises that don't resolve. Is it possible to introduce a sane default timeout? I guess it's not for this issue. Just food for thought.

Copy link
Contributor

Choose a reason for hiding this comment

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

For event publish, there is a timeout option that defaults to 30 seconds.

README.md Outdated

When handling an event it is sometimes useful to be able to send information back to the source of the event. For example, `ftrack.location.request-resolve` would expect a resolved path to be sent back.

You can craft a custom reply event if you want, but an easier way is just to return the appropriate data from your handler. Any value different from *null* or *undefined* will be automatically sent as a reply:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd love a note here referencing the fact that this implicitly uses the publishReply function (described further down).

I personally missed this the first time reading the documentation, and it lead me to believe that it was only possible to respond to events synchronously directly in the callback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We actually don't describe publishReply currently. I don't know enough about what a believable example would be to write that section, but I've added a short mention of publishReply here.


#### Subscription expressions

The JavaScript API currently only support expressions on the format `topic=value` including wildcard support `topic=ftrack.*`, and more complex expressions such as filtering based on event source or data are not supported.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this true? Wasn't wildcard support added recently? I believe at least subscribing to all events is possible. We are doing that now.

Copy link
Contributor

Choose a reason for hiding this comment

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

The more complex expressions, like filtering on source are still not supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, wildcards is explicitly stated as supported. If the wording is confusing, please suggest an alternate wording.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. I misread it lol. It's actually really clear. Just a brain fart.

README.md Outdated
#### Target expression

Targeted events will invoke all subscribers of the topic, not just those
matching the target expression-
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be a period instead of a dash at the end here.

session.eventHub.publishAndWaitForReply(event, { timeout: 5 }).then(onReply, onError);
```

### Limitations
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 all of these limitations should have issues created for them. Would make it easier for us to contribute with fixes.

Copy link
Contributor

@lucaas lucaas left a comment

Choose a reason for hiding this comment

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

Nice work! Feel free to merge as-is or address comments first as you wish.

We will iterate on this, but need some basic info in place in the meantime.


#### Subscription expressions

The JavaScript API currently only support expressions on the format `topic=value` including wildcard support `topic=ftrack.*`, and more complex expressions such as filtering based on event source or data are not supported.
Copy link
Contributor

Choose a reason for hiding this comment

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

The more complex expressions, like filtering on source are still not supported.

README.md Outdated

### Error handling

A lot of the methods involve promises that might not always be resolved for various reasons. The examples will not have any error handling to keep them short, but make sure you have proper error handling when using the `EventHub`.
Copy link
Contributor

Choose a reason for hiding this comment

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

For event publish, there is a timeout option that defaults to 30 seconds.

Copy link
Contributor

@jimmycallin jimmycallin left a comment

Choose a reason for hiding this comment

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

Sweet!

Found one more file we can remove: MANIFEST.in

We should redirect the old documentation URL to this repository as well.

@gismya gismya merged commit b1b745e into main Sep 15, 2023
@gismya gismya deleted the update-documentation branch September 15, 2023 11:34
@gismya
Copy link
Contributor Author

gismya commented Sep 15, 2023

We should redirect the old documentation URL to this repository as well.

Yes. @lucaas is on it.

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.

4 participants