Skip to content
This repository was archived by the owner on Jan 7, 2025. It is now read-only.

rebase and update IDL changes - #18

Merged
Sapples merged 5 commits into
mainfrom
edx-180-rebase
Aug 1, 2022
Merged

rebase and update IDL changes#18
Sapples merged 5 commits into
mainfrom
edx-180-rebase

Conversation

@Sapples

@SapplesSapples commented Jul 27, 2022

Copy link
Copy Markdown
Contributor

Based on: EDX-180
Follow up/replacement from old PR to be closed: #5

  • Add changes as per #1427
  • Add changes as per #1444
  • Add changes as per #1453
  • Add changes as per #1460 (No changes to IDL)
  • Add changes as per #1462
  • Add changes as per #1464
  • Add changes as per #1465

@SapplesSapples self-assigned this Jul 27, 2022
@Sapples
Sapples requested a review from m-hulbertJuly 27, 2022 15:35

@m-hulbertm-hulbert left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Quite a few comments on this one, sorry. Happy to walk through it with you though if that would help.

In addition there's some things that were missing from the linked PRs, as well as one change I think is in a separate PR that wasn't listed in the original JIRA but should be covered as part of this:

  • Update Rest and Realtime to RestClient and RealtimeClient and all associated links as per https://github.com/ably/docs/pull/1459/files. Be aware of any mention of "REST client" or "Realtime client" as well as any links to those classes.
  • Auth should have a tokenDetails property as per https://github.com/ably/docs/pull/1462/files
  • I think the description for extras in the Message class needs to be updated due to the message interactions implementation.
  • ably/docs#1499 added clientId to message filters. This needs to be added.
  • The cipher property needs updating in ChannelOptions as per ably/docs#1464
  • I made a comment about making deprecated properties consistent in a few places, so happy to help with this too!

Comment threaddescriptions.md Outdated
Comment threaddescriptions.md Outdated
Comment threaddescriptions.md Outdated
Comment threaddescriptions.md Outdated
Comment threaddescriptions.md Outdated
Comment threaddescriptions.md Outdated
Comment threaddescriptions.md Outdated
Comment threaddescriptions.md Outdated
Comment threaddescriptions.md Outdated
Comment threaddescriptions.md
@Sapples
Sapples requested a review from m-hulbertJuly 29, 2022 11:36

@m-hulbertm-hulbert left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  • All the references to Stats.* objects should be in backticks.
  • I think the stats descriptions need checking over again. I think the descriptions have been carried through for them all about the "count and byte value of messages", however this can be quite nested. Taking Stats.MessageDirections as an example, two of the properties are Stats.MessageTraffic and the other is Stats.MessageTypes yet they all have the same description. This applies to all the stats classes.

Comment threaddescriptions.md Outdated
Comment threaddescriptions.md Outdated
Comment threaddescriptions.md Outdated
Comment threaddescriptions.md Outdated
Comment threaddescriptions.md Outdated
Comment threaddescriptions.md Outdated
Comment threaddescriptions.md Outdated
Comment threaddescriptions.md Outdated
Comment threaddescriptions.md Outdated
Comment threaddescriptions.md Outdated

@m-hulbertm-hulbert left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The cipher property needs updating in ChannelOptions as per ably/docs#1464 still please.

Comment threaddescriptions.md Outdated
Comment threaddescriptions.md Outdated
@Sapples
Sapples requested a review from m-hulbertJuly 29, 2022 17:27

@m-hulbertm-hulbert left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just a couple of styling points and a few stats suggestions!

Comment threaddescriptions.md Outdated
Comment threaddescriptions.md Outdated
Comment threaddescriptions.md Outdated
Comment threaddescriptions.md Outdated
Comment threaddescriptions.md Outdated
Comment threaddescriptions.md Outdated
Comment threaddescriptions.md Outdated
Comment threaddescriptions.md Outdated
Comment threaddescriptions.md Outdated
Comment threaddescriptions.md Outdated
@Sapples
Sapples requested a review from m-hulbertAugust 1, 2022 10:40

@m-hulbertm-hulbert left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

@Sapples
Sapples merged commit cb851c4 into mainAug 1, 2022
@m-hulbert
m-hulbert deleted the edx-180-rebase branch August 2, 2022 08:26
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@Sapples@m-hulbert@StevenAppleby