Handle blank password in basic auth. #690

Merged
merged 2 commits into from Oct 24, 2013

Projects

None yet

3 participants

@diversario
Contributor

Fixes #681.

@nylen
Member
nylen commented Oct 24, 2013

My only issue with this is that now options.auth.pass will behave differently from options.auth.password. However, there's already a similar issue with options.auth.user vs options.auth.username, so I'm not sure how much this matters. @mikeal, care to comment?

@diversario
Contributor

@nylen that's true. Are password and username legacy options? If so it would make sense to map them on the shorter fields, if they're present. I can't imagine that people use both at the same time. So, something like

if (Object.prototype.hasOwnProperty.call(options.auth, 'username')) options.auth.user = options.auth.username
if (Object.prototype.hasOwnProperty.call(options.auth, 'password')) options.auth.pass = options.auth.password

No need to care about legacy options after this.

@nylen
Member
nylen commented Oct 24, 2013

I agree. Personally I would have gone with typeof options.auth.username != 'undefined', but I'd merge it either way.

I put the alias names in there so that you could use either variant, without having to refer back to the docs to remember the naming convention.

@diversario
Contributor

How about that? Last commit?

@mikeal mikeal merged commit 7fd5498 into request:master Oct 24, 2013

1 check was pending

default The Travis CI build is in progress
Details
@nylen
Member
nylen commented Oct 24, 2013

Much cleaner, thanks!

@mikeal
Member
mikeal commented Oct 24, 2013

for some reason Travis is complaining about tests failing with optionals enabled/disbled or something. i have no idea what that means.

@diversario
Contributor

Tests pass locally (for me, at least).

@mikeal
Member
mikeal commented Oct 24, 2013

me too :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment