Skip to content

urls [nfc]: Represent realm URL as Uri object, not String #68

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 4 commits into from
Apr 20, 2023

Conversation

chrisbobbe
Copy link
Collaborator

No description provided.

@chrisbobbe chrisbobbe requested a review from gnprice April 17, 2023 23:40
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning this up! Looks good modulo one comment.

Comment on lines +56 to +74
final url = auth.realmUrl.replace(
path: "/api/v1/$route", queryParameters: encodeParameters(params));
Copy link
Member

Choose a reason for hiding this comment

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

This version is implicitly assuming that realmUrl.fragment is empty.

Similarly the code in post and in the two other routes is assuming query and fragment are both empty. (In those, the old code was also assuming that, though differently so.)

That assumption is fine, but let's make it explicit with an assert. Can put it on the Auth constructor:

  Auth({required this.realmUrl, required this.email, required this.apiKey})
    : assert(realmUrl.query.isEmpty && realmUrl.fragment.isEmpty);

@chrisbobbe
Copy link
Collaborator Author

Thanks for the review! Revision pushed, resolving a conflict with #63.

@gnprice gnprice merged commit 9152167 into zulip:main Apr 20, 2023
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.

2 participants