-
Notifications
You must be signed in to change notification settings - Fork 23
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
Added [WebAuthenticator], which allows for user authentication through a browser #5
Conversation
https://www.reddit.com/api/v1/authorize. Authentication + authorization have been tested, but nothing else at the moment.
lib/src/auth.dart
Outdated
Map<String, String> revokeAccess = new Map<String, String>(); | ||
revokeAccess[kTokenKey] = accessToken; | ||
revokeAccess[kTokenTypeHintKey] = 'access_token'; | ||
List<Map> tokens = new List<Map>(); |
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.
Avoid LHS type, and use literals when possible. For example
final tokens = <Map>[];
lib/src/auth.dart
Outdated
// Retrieve the client ID and secret. | ||
String clientId = _grant.identifier; | ||
String clientSecret = _grant.secret; | ||
for (int i = 0; i < tokens.length; i++) { |
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.
Prefer for-in loops when the index isn't used for anything other than indexing the collection.
for (var token in tokens) {
...
}
lib/src/auth.dart
Outdated
/// https://github.com/reddit/reddit/wiki/OAuth2-App-Types for descriptions of | ||
/// valid app types. | ||
class WebAuthenticator extends Authenticator { | ||
static WebAuthenticator Create( |
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.
Only type names should begin with uppercase letters in Dart: https://www.dartlang.org/guides/language/effective-dart/style#do-name-types-using-uppercamelcase
lib/src/auth.dart
Outdated
String userInfo = '$clientId:$clientSecret'; | ||
// TODO(bkonyi) we shouldn't have hardcoded urls like this. Move to common | ||
// file with all API related strings. | ||
Uri path = Uri.parse(r'https://www.reddit.com/api/v1/revoke_token'); |
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.
Avoid LHS type unless it's unclear from context or type inference doesn't infer the desired type.
lib/src/auth.dart
Outdated
Future _requestToken(Map<String, String> accountInfo) async { | ||
// Retrieve the client ID and secret. | ||
String clientId = _grant.identifier; | ||
String clientSecret = _grant.secret; | ||
String userInfo = null; | ||
|
||
if ((clientId != null) && (clientSecret != null)) { |
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.
nit: Unnecessary parentheses. While generally useful if precedence rules are unclear, I'd argue this condition is simple enough the parentheses don't improve clarity.
lib/src/auth.dart
Outdated
_client = await _client.refreshCredentials(); | ||
} | ||
|
||
Uri _redirect; |
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.
nit: Fields are generally declared at the top of your class before constructors.
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 great, most of my comments are just nit-picky style guidelines which are largely subjective.
I'd recommend using final
on local variable declarations. It's a good defensive coding tool that helps prevent bugs and helps readers reason about your code. There's a lint available, but it does have some false positives: http://dart-lang.github.io/linter/lints/prefer_final_locals.html
lib/src/auth.dart
Outdated
String clientSecret = _grant.secret; | ||
final clientId = _grant.identifier; | ||
final clientSecret = _grant.secret; | ||
String userInfo = null; |
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.
All declarations are implicitly null
. There's a lint for this: http://dart-lang.github.io/linter/lints/avoid_init_to_null.html 😉
…h a browser (#5) Added [WebAuthenticator], which allows for user authentication at https://www.reddit.com/api/v1/authorize. Authentication + authorization have been tested, but nothing else at the moment.
Added [WebAuthenticator], which allows for user authentication at https://www.reddit.com/api/v1/authorize. Authentication + authorization have been tested, but nothing else at the moment.