Make the oauth2 lib handle OAuth2 401 errors.

Make the oauth2 lib handle OAuth2 401 errors.


BUG=6813
Committed: https://code.google.com/p/dart/source/detail?r=15713

[Bob Nystrom, 2012-12-04 01:41:48]

https://codereview.chromium.org/11316325/diff/1/pkg/oauth2/lib/src/utils.dart
File pkg/oauth2/lib/src/utils.dart (right):

https://codereview.chromium.org/11316325/diff/1/pkg/oauth2/lib/src/utils.dart...
pkg/oauth2/lib/src/utils.dart:68: /** A pair of values. */
If you're creating this type anyway, how about just creating a more specific and
 semantic AuthenticateHeader class?

https://codereview.chromium.org/11316325/diff/1/pkg/oauth2/lib/src/utils.dart...
pkg/oauth2/lib/src/utils.dart:108: match = regexp.firstMatch(paramString);
Can you use allMatches() here?

https://codereview.chromium.org/11316325/diff/1/pkg/oauth2/test/client_test.dart
File pkg/oauth2/test/client_test.dart (right):

https://codereview.chromium.org/11316325/diff/1/pkg/oauth2/test/client_test.d...
pkg/oauth2/test/client_test.dart:175: '"Something is terribly wrong.", ';
Indent +2 more.

[nweiz, 2012-12-04 01:51:35]

https://codereview.chromium.org/11316325/diff/1/pkg/oauth2/lib/src/utils.dart
File pkg/oauth2/lib/src/utils.dart (right):

https://codereview.chromium.org/11316325/diff/1/pkg/oauth2/lib/src/utils.dart...
pkg/oauth2/lib/src/utils.dart:68: /** A pair of values. */
On 2012/12/04 01:41:48, Bob Nystrom wrote:
> If you're creating this type anyway, how about just creating a more specific
and
>  semantic AuthenticateHeader class?

Why would I create a specific type when I could make a more general one? I think
there's a lot of benefit to it being available for use in case anything else
internal needs to use it, and in being copy-paste identical with the version in
pub (well, given that it's not in the core libraries). This also makes the
pairOf function in utils_test nicer than it would be otherwise.

https://codereview.chromium.org/11316325/diff/1/pkg/oauth2/lib/src/utils.dart...
pkg/oauth2/lib/src/utils.dart:108: match = regexp.firstMatch(paramString);
On 2012/12/04 01:41:48, Bob Nystrom wrote:
> Can you use allMatches() here?

No. I need to enforce that each match follows directly after the one before it,
and that the first one starts at the beginning of the string.

https://codereview.chromium.org/11316325/diff/1/pkg/oauth2/test/client_test.dart
File pkg/oauth2/test/client_test.dart (right):

https://codereview.chromium.org/11316325/diff/1/pkg/oauth2/test/client_test.d...
pkg/oauth2/test/client_test.dart:175: '"Something is terribly wrong.", ';
On 2012/12/04 01:41:48, Bob Nystrom wrote:
> Indent +2 more.

Done.

[Bob Nystrom, 2012-12-04 01:59:42]

https://codereview.chromium.org/11316325/diff/1/pkg/oauth2/lib/src/utils.dart
File pkg/oauth2/lib/src/utils.dart (right):

https://codereview.chromium.org/11316325/diff/1/pkg/oauth2/lib/src/utils.dart...
pkg/oauth2/lib/src/utils.dart:68: /** A pair of values. */
On 2012/12/04 01:51:35, nweiz wrote:
> On 2012/12/04 01:41:48, Bob Nystrom wrote:
> > If you're creating this type anyway, how about just creating a more specific
> and
> >  semantic AuthenticateHeader class?
> 
> Why would I create a specific type when I could make a more general one?

Why make types at all: They help clarify intent and semantics. Especially where
the result of parseAuthenticateHeader is being used, there's no nearby type
annotation to help understand what kind of object it's returning.

> I think
> there's a lot of benefit to it being available for use in case anything else
> internal needs to use it, and in being copy-paste identical with the version
in
> pub (well, given that it's not in the core libraries). 

I'm sympathetic here. If Dart actually had tuples, I'd be in favor of using
them. But it doesn't, and the prevailing style seems to be pretty... nominal. In
this case, using a pair doesn't save you anything because you still have to
define the pair class. Given that, I'd rather see it have a name and nicely
named fields.

Actually, even better would be to move parseAuthenticateHeader into it as a
constructor:

class AuthenticateHeader {
  final String scheme;
  final Map<> fields;

  factory AuthenticateHeader.parse() { ... }
}

WDYT?

> This also makes the
> pairOf function in utils_test nicer than it would be otherwise.

https://codereview.chromium.org/11316325/diff/1/pkg/oauth2/lib/src/utils.dart...
pkg/oauth2/lib/src/utils.dart:108: match = regexp.firstMatch(paramString);
On 2012/12/04 01:51:35, nweiz wrote:
> On 2012/12/04 01:41:48, Bob Nystrom wrote:
> > Can you use allMatches() here?
> 
> No. I need to enforce that each match follows directly after the one before
it,
> and that the first one starts at the beginning of the string.

OK, that's what I figured. Just checking.

[nweiz, 2012-12-04 21:39:26]

https://codereview.chromium.org/11316325/diff/1/pkg/oauth2/lib/src/utils.dart
File pkg/oauth2/lib/src/utils.dart (right):

https://codereview.chromium.org/11316325/diff/1/pkg/oauth2/lib/src/utils.dart...
pkg/oauth2/lib/src/utils.dart:68: /** A pair of values. */
On 2012/12/04 01:59:42, Bob Nystrom wrote:
> On 2012/12/04 01:51:35, nweiz wrote:
> > On 2012/12/04 01:41:48, Bob Nystrom wrote:
> > > If you're creating this type anyway, how about just creating a more
specific
> > and
> > >  semantic AuthenticateHeader class?
> > 
> > Why would I create a specific type when I could make a more general one?
> 
> Why make types at all: They help clarify intent and semantics. Especially
where
> the result of parseAuthenticateHeader is being used, there's no nearby type
> annotation to help understand what kind of object it's returning.
> 
> > I think
> > there's a lot of benefit to it being available for use in case anything else
> > internal needs to use it, and in being copy-paste identical with the version
> in
> > pub (well, given that it's not in the core libraries). 
> 
> I'm sympathetic here. If Dart actually had tuples, I'd be in favor of using
> them. But it doesn't, and the prevailing style seems to be pretty... nominal.
In
> this case, using a pair doesn't save you anything because you still have to
> define the pair class. Given that, I'd rather see it have a name and nicely
> named fields.
> 
> Actually, even better would be to move parseAuthenticateHeader into it as a
> constructor:
> 
> class AuthenticateHeader {
>   final String scheme;
>   final Map<> fields;
> 
>   factory AuthenticateHeader.parse() { ... }
> }
> 
> WDYT?
> 
> > This also makes the
> > pairOf function in utils_test nicer than it would be otherwise.
> 

Done. I'm still not convinced this is better, but I don't think it much either
way.

[Bob Nystrom, 2012-12-04 21:47:03]

On 2012/12/04 21:39:26, nweiz wrote:
> https://codereview.chromium.org/11316325/diff/1/pkg/oauth2/lib/src/utils.dart
> File pkg/oauth2/lib/src/utils.dart (right):
> 
>
https://codereview.chromium.org/11316325/diff/1/pkg/oauth2/lib/src/utils.dart...
> pkg/oauth2/lib/src/utils.dart:68: /** A pair of values. */
> On 2012/12/04 01:59:42, Bob Nystrom wrote:
> > On 2012/12/04 01:51:35, nweiz wrote:
> > > On 2012/12/04 01:41:48, Bob Nystrom wrote:
> > > > If you're creating this type anyway, how about just creating a more
> specific
> > > and
> > > >  semantic AuthenticateHeader class?
> > > 
> > > Why would I create a specific type when I could make a more general one?
> > 
> > Why make types at all: They help clarify intent and semantics. Especially
> where
> > the result of parseAuthenticateHeader is being used, there's no nearby type
> > annotation to help understand what kind of object it's returning.
> > 
> > > I think
> > > there's a lot of benefit to it being available for use in case anything
else
> > > internal needs to use it, and in being copy-paste identical with the
version
> > in
> > > pub (well, given that it's not in the core libraries). 
> > 
> > I'm sympathetic here. If Dart actually had tuples, I'd be in favor of using
> > them. But it doesn't, and the prevailing style seems to be pretty...
nominal.
> In
> > this case, using a pair doesn't save you anything because you still have to
> > define the pair class. Given that, I'd rather see it have a name and nicely
> > named fields.
> > 
> > Actually, even better would be to move parseAuthenticateHeader into it as a
> > constructor:
> > 
> > class AuthenticateHeader {
> >   final String scheme;
> >   final Map<> fields;
> > 
> >   factory AuthenticateHeader.parse() { ... }
> > }
> > 
> > WDYT?
> > 
> > > This also makes the
> > > pairOf function in utils_test nicer than it would be otherwise.
> > 
> 
> Done. I'm still not convinced this is better, but I don't think it much either
> way.

It makes me happier!

[Bob Nystrom, 2012-12-04 21:47:10]

LGTM.