Skip to content

Conversation

@chaosct
Copy link
Contributor

@chaosct chaosct commented Dec 16, 2015

Closes #211

Copy link
Member

Choose a reason for hiding this comment

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

Let's tighten this up a little bit if you don't mind. =)

Firstly, we can remove the auth or portion of this line: we already know auth is false, that's how we got here, so no need to check it again.

Secondly, let's add a line inside this block that asserts that password is truthy.

Thirdly and finally, let's remove the parentheses around username in the conditional: they're not needed. =)

@Lukasa
Copy link
Member

Lukasa commented Dec 16, 2015

Thanks! Some small notes. =)

@chaosct
Copy link
Contributor Author

chaosct commented Dec 16, 2015

Should I make those changes? Or are you already on it?

@Lukasa
Copy link
Member

Lukasa commented Dec 16, 2015

I'm happy for you to do it. =)

@chaosct
Copy link
Contributor Author

chaosct commented Dec 16, 2015

For the password, I guess an empty password is OK? I'll then compare it with is None.

And with this code, if username is empty string, it won't use Basic Auth, but I am not sure if empty string is a valid username or password.

@Lukasa
Copy link
Member

Lukasa commented Dec 16, 2015

@chaosct I don't believe an empty string is a valid username.

@chaosct
Copy link
Contributor Author

chaosct commented Dec 16, 2015

Let me know if more changes are needed

Copy link
Member

Choose a reason for hiding this comment

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

Heh, reading this now I find the operator precedence confusing. Let's do if (not auth) and username just to indicate what's going on here.

@Lukasa
Copy link
Member

Lukasa commented Dec 16, 2015

Beautiful, almost perfect. One tiny change and then it's good to go!

@chaosct
Copy link
Contributor Author

chaosct commented Dec 16, 2015

Done!

@Lukasa
Copy link
Member

Lukasa commented Dec 16, 2015

\o/ Perfect! Thanks @chaosct!

Lukasa added a commit that referenced this pull request Dec 16, 2015
Preventing sending Basic Auth headers with "None:None"
@Lukasa Lukasa merged commit 1c15f31 into requests:master Dec 16, 2015
@citizenken
Copy link

Can a new release be created to include these changes please? Installing master straight from Github is not ideal.

@sigmavirus24
Copy link
Contributor

@citizenken it would appear this results in a new bug so creating a release right now does not seem ideal

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.6%) to 85.437% when pulling 9b124ef on chaosct:master into f1d5331 on requests:master.

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.

5 participants