Parameterize the ApiConnection in getServerSettings. #103
Conversation
There was a problem hiding this comment.
Thanks @samwightt for the contribution!
See comments below. Before we can finish reviewing your main change, we'll need a revision that has a clean diff which doesn't make unrelated formatting changes.
The first commit, making an API update for a change in Flutter main, looks good. I'll merge it shortly with small stylistic tweaks.
| } finally { | ||
| connection.close(); | ||
| apiConnection.close(); |
There was a problem hiding this comment.
The responsibility for closing the connection should stay with the same code that's opening it. So when this function starts getting the ApiConnection from its caller, this part should move to the caller too.
For similar code to compare to, see the other API-endpoint bindings in other files of this lib/api/route/ directory.
| @@ -16,15 +16,13 @@ part 'realm.g.dart'; | |||
| // See thread, and the zulip-mobile code and chat thread it links to: | |||
| // https://github.com/zulip/zulip-flutter/pull/55#discussion_r1160267577 | |||
| Future<GetServerSettingsResult> getServerSettings({ | |||
| required Uri realmUrl, | |||
| required ApiConnection apiConnection, | |||
There was a problem hiding this comment.
This interface should align with the other functions in this directory that do a similar job of providing a binding to an API endpoint: a positional parameter named connection, rather than a named parameter named apiConnection.
| return ServerUrlParseResult.error(ServerUrlValidationError.unsupportedSchemeZulip); | ||
| } else if (url != null && url.hasScheme && url.scheme != 'http' && url.scheme != 'https') { | ||
| return ServerUrlParseResult.error(ServerUrlValidationError.unsupportedSchemeOther); | ||
| return ServerUrlParseResult.error( | ||
| ServerUrlValidationError.unsupportedSchemeZulip); | ||
| } else if (url != null && | ||
| url.hasScheme && | ||
| url.scheme != 'http' && | ||
| url.scheme != 'https') { |
There was a problem hiding this comment.
It looks like this code wasn't something you intended to make changes to. So it should remain unchanged in this commit.
Like the Flutter repo itself, we don't use dartfmt.
Because there's a lot of this kind of change in this file, it's difficult to isolate the substantive changes in order to review those. So before we can finish reviewing this PR, you'll need to make a revision with a clean diff that doesn't make unrelated formatting changes.
lib/widgets/sticky_header.dart
Outdated
| with SlottedContainerRenderObjectMixin<StickyHeaderSlot> { | ||
| with SlottedContainerRenderObjectMixin<StickyHeaderSlot, RenderBox> { |
Done: |
gnprice
force-pushed
the
sw/parameterize-api-connection
branch
from
3d78afe
to
d79709a
Compare
|
I've rebased this PR branch on top of the now-merged version of your first commit, just because there'd otherwise be a small merge conflict for you to resolve. |
|
@gnprice Thanks for the feedback! Sorry, didn't realize there were so many formatting problems 😅 I'll take a look at this tomorrow and rebase + fix things :) |
|
@samwightt Are you still planning to return to this PR? If not, no worries; just came across it in our queue. |
|
@gnprice Agh sorry I'm not! Sorry about getting back to this so late. |
|
No worries! Thanks for the update. |
Parameterizes the ApiConnection in getServerSettings so that it can now be tested. Also fixes issue with
SlottedContainerRenderObjectMixin, as it now requires two type parameters on Fluttermain.