-
Notifications
You must be signed in to change notification settings - Fork 128
Conversation
this.tokenEndpoint, | ||
{this.secret, bool basicAuth: true, http.Client httpClient}) | ||
this.identifier, this.authorizationEndpoint, this.tokenEndpoint, | ||
{this.secret, bool basicAuth: true, http.Client httpClient}) |
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.
Please don't include unrelated formatting changes.
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.
Ok, I won't. Sorry, didn't know.
Uri getAuthorizationUrl(Uri redirect, {Iterable<String> scopes, | ||
String state}) { | ||
Uri getAuthorizationUrl(Uri redirect, | ||
{Iterable<String> scopes, String state, String delimiter: ' '}) { |
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.
Document the delimiter parameter, and assign the default in the body rather than in the parameter list (putting it in the parameter list makes it impossible for users to express "use the default" without explicitly encoding that default).
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.
Will do.
@nex3 Sorry for not updating this in a long time, but I've now made the requested changes. |
One more thing (sorry I didn't catch this in the first review!): I think for |
Okay, that works. I'll update it tomorrow, thanks! |
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
Not really sure why the CLA thing went away, but I've signed it before. |
It looks like GitHub thinks that dd57b59 has an invalid author email. Did you commit with a different email than you normally use? |
No, it's the same one; however, I recently bought a new computer, so maybe that has something to do with it. It's times like these when I realize how little I actually know about Git, haha. |
Looking at the commits locally, it looks like your most recent one has your email as the committer name, and |
CLAs look good, thanks! |
Okay, so I was able to change my credentials locally. Had to rewrite the commit history and change my e-mail as well. CLA approval is back, woohoo! |
/// The scope strings will be separated by the provided [delimiter]. This | ||
/// defaults to `" "`, the OAuth2 standard, but some APIs (such as Facebook's) | ||
/// use non-standard delimiters. | ||
final String delimiter; |
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.
This should be private, and the constructor parameter should be documented independently.
@@ -148,7 +153,7 @@ class AuthorizationCodeGrant { | |||
}; | |||
|
|||
if (state != null) parameters['state'] = state; | |||
if (!scopes.isEmpty) parameters['scope'] = scopes.join(' '); | |||
if (!scopes.isEmpty) parameters['scope'] = scopes.join(delimiter ?? ' '); |
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.
It would simplify the data model to assign the field to ' '
if the parameter isn't passed, rather than checking on every use.
Credentials handleAccessTokenResponse( | ||
http.Response response, | ||
Uri tokenEndpoint, | ||
DateTime startTime, | ||
List<String> scopes) { | ||
List<String> scopes, | ||
{String delimiter}) { |
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.
It's not worth making this optional since it's only called internally.
@@ -56,13 +61,13 @@ Future<Client> resourceOwnerPasswordGrant( | |||
} | |||
} | |||
|
|||
if (scopes != null && !scopes.isEmpty) body['scope'] = scopes.join(' '); | |||
if (scopes != null && !scopes.isEmpty) body['scope'] = scopes.join(delimiter ?? ' '); |
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.
Similarly to the above, just do delimiter ??= ' '
at the top of the function.
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.
Okay, just pushed commits to correct these. Let me know if they're up to your expectations!
# Conflicts: # lib/src/authorization_code_grant.dart
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
Made the changes, but I'm an idiot and keep running into the CLA thing. Give me 2 minutes to fix it. Hopefully I'll have it done by the time you see this. |
CLAs look good, thanks! |
The code looks good! Can you add tests? |
Sure. Which tests? |
We should test every place the scope parameter escapes the package. |
All right, I'll add some, and then just let me know if I'm missing anything! |
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
CLAs look good, thanks! |
Looks good, thanks for iterating on this! |
Awesome, thanks for the merge! This should fix dart-lang/tools#308. Now I'll get started on dart-lang/tools#303. |
Fixes the following issues: