Skip to content

Add RSL8, RSL8a lifecycle channel status#365

Merged
owenpearson merged 4 commits intomainfrom
issue-362
May 31, 2022
Merged

Add RSL8, RSL8a lifecycle channel status#365
owenpearson merged 4 commits intomainfrom
issue-362

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented May 23, 2022 •
edited by ghost
Loading

Resolves #362

RSL8, RSL8a

Łukasz Śliwa added 2 commits May 23, 2022 22:31
@ghost ghost requested a review from owenpearson May 23, 2022 20:39
@ghost ghost self-assigned this May 23, 2022
@ghost ghost marked this pull request as ready for review May 23, 2022 22:14
owenpearson
owenpearson requested changes May 27, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@owenpearson owenpearson left a comment

Choose a reason for hiding this comment

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

It looks like Channel.status doesn't actually make the request to the REST API here, there should be a call to client.get, see the example from ably-go. Also we need an integration test for this, see the ably-js test for an example.

@ghost
Copy link
Copy Markdown
Author

ghost commented May 30, 2022 •
edited by ghost
Loading

It looks like Channel.status doesn't actually make the request to the REST API here, there should be a call to client.get, see the example from ably-go. Also we need an integration test for this, see the ably-js test for an example.

It makes a GET request via REST API using client.get, see here https://github.com/ably/ably-ruby/pull/365/files#diff-8bd42be102e30307cc846b11e2c55a7ebccecbd260caf96bd49ff70d0eabd3b6R168
Acceptance/integration test is also implemented, see here https://github.com/ably/ably-ruby/pull/365/files#diff-a281ddaea4796c02a5a7ca34ecfa7361b1797228af6cff2cfd26eb794af764fd

owenpearson
owenpearson approved these changes May 30, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@owenpearson owenpearson left a comment

Choose a reason for hiding this comment

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

@lukaszsliwa That's so weird, not sure how I missed both of those when I reviewed this before 😅 anyway, LGTM now

owenpearson
owenpearson requested changes May 30, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@owenpearson owenpearson left a comment

Choose a reason for hiding this comment

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

Actually, before we merge this would you mind adding some rubydoc comments to the new models? You can use the descriptions from https://github.com/ably/docs/blob/main/content/client-lib-development-guide/features.textile#channeldetails

@ghost
Copy link
Copy Markdown
Author

ghost commented May 30, 2022

Actually, before we merge this would you mind adding some rubydoc comments to the new models? You can use the descriptions from https://github.com/ably/docs/blob/main/content/client-lib-development-guide/features.textile#channeldetails

Okey, I will add some more comments

owenpearson
owenpearson approved these changes May 31, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@owenpearson owenpearson 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 👍

@owenpearson owenpearson merged commit 0f88cbe into main May 31, 2022
@owenpearson owenpearson deleted the issue-362 branch May 31, 2022 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add support to get channel lifecycle status

1 participant

@owenpearson