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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle empty EHLO and HELO commands #112
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests look great, just requested a minor tweak on implementation and then you're good to go.
ehlo_as = ehlo_as.decode('utf-8') | ||
reply = Reply('250', 'Hello '+ehlo_as) | ||
reply.enhanced_status_code = False | ||
self._call_custom_handler('EHLO', reply, ehlo_as) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd rather see this implemented as if/elif clauses at the top of the handlers, e.g.
if not self.bannered:
bad_sequence.send(self.io)
return
elif not ehlo_as:
bad_arguments.send(self.io)
return
Do you want that I rewrite git history in order to melt commits before merging? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to squash commits, I'll do that on merge. This looks great, thanks!
Related to #111.
I also add tests.
Feedbacks are welcome! 馃槃