Skip to content
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

small fix #70

Merged
merged 5 commits into from
Apr 30, 2017
Merged

small fix #70

merged 5 commits into from
Apr 30, 2017

Conversation

kozzztik
Copy link
Contributor

small fix about fields usage - self.transport and self._connection_closed duplicate each other.

Copy link
Contributor

@warsaw warsaw left a comment

Choose a reason for hiding this comment

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

Very nice clean up thank you! I have just one minor coding style comment to consider.

aiosmtpd/smtp.py Outdated
@@ -195,7 +193,7 @@ def _handle_client(self):
log.info('%r handling connection', self.session.peer)
yield from self.push(
'220 {} {}'.format(self.hostname, self.__ident__))
while not self._connection_closed: # pragma: no branch
while self.transport: # pragma: no branch
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you make the check here and below a little more explicit? Rather than checking truthiness, please use while self.transport is not None

@warsaw warsaw added the tech-debt Things that needs to be tidied up to avoid being bitten in the future... label Apr 19, 2017
@warsaw warsaw added this to the 1.0 milestone Apr 19, 2017
@Mortal Mortal mentioned this pull request Apr 19, 2017
@kozzztik
Copy link
Contributor Author

Updated.

@warsaw warsaw merged commit 2f7c8f4 into aio-libs:master Apr 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tech-debt Things that needs to be tidied up to avoid being bitten in the future...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants