Skip to content

Conversation

@martino
Copy link

@martino martino commented May 11, 2017

No description provided.

Copy link
Collaborator

@vad vad left a comment

Choose a reason for hiding this comment

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

requires a small refactoring to avoid code duplication

def block_smtp(whitelist):
def whitelisted(self, host, *args, **kwargs):
if isinstance(host, basestring) and host not in whitelist:
try:
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you move this to the module level and reuse it in block_http?

Copy link

@adamchainz adamchainz left a comment

Choose a reason for hiding this comment

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

Why not use six?

@vad
Copy link
Collaborator

vad commented May 29, 2017

@adamchainz there's no six dependency ATM. It's only a few lines of code to implement that check, do you think it's worth it?

@adamchainz
Copy link

six is in pretty much every environment these days, I consider it basically part of the standard library. It's also easier to find and remove six when moving from 2/3 compatibility to just 3, in 2020 or sooner 😉 .

@vad
Copy link
Collaborator

vad commented May 29, 2017

👍 to "it'll will make removal easier"

@do3cc
Copy link
Collaborator

do3cc commented Jun 7, 2017

Six would be a more expicit way to say: This conditional code is here because of 2-3 compatibility. It is easier to find all instances and remove them when we stop supporting python 2. Which won't be soon.

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.

4 participants