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
Refactor of ClientOptions #348
Conversation
…or of ClientOptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall everything looks good but I think that one thing is missing - update in the README and Ably docs 😉 Since we've deprecated the fromKey
I believe we shouldn't suggest it in our usage guides.
I took a quick look and found at least 3 places (but there may be more) in which we're using the deprecated fromKey
approach:
- project README (let's update it as a part of this PR)
- Ably quickstart guide (probably we should inform the docs team about it, what's the best way to do this @QuintinWillison ? 🤔 )
- plugin details on pub.dev (probably this is taken from the README and will be updated automatically with the next release, right?)
Good catch! I forgot about README, I'm going to update it in a while, but I'm not sure about the docs and quickstart guide - I've checked the spec before this refactor and there's no reference to You're correct with pub.dev descriptions, it's generated from the readme file 🙂 I've fixed these references in 8a7666d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now, thanks 😉 The only thing left is the fromKey()
reference in the Ably docs but this is outside of this PR scope, as the change should probably be made after this PR is released in a new ably-flutter
version 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, albeit admitting I have skim read much of the contents.
This is a part of #61, including changes only for
ClientOptions
andAuthOptions
, without moving the fields as private. Main changes:AuthOptions
andClientOptions
to use named parametersfromKey
methods, since the object can be now created with constructor invocationClientOptions
was created with public parameter settersAlthough this PR is fairly large, most of the changes were done in integration testing, and none of the changes are breaking in any way - users should be able to migrate without any issues. Moving the variables as private is a breaking change, so I decided to implement this later, in a separate PR, to give users some more time to adopt these changes before we deprecate the public fields