Conversation
|
Thanks for this. It looks promising. I've had a crazy week so I haven't been able to look at the changes properly yet but I will soon. |
mjs
left a comment
mjs
left a comment
There was a problem hiding this comment.
Thanks so much for this. There's more that can be cleaned up but this is a big enough first step and I think it's the right approach.
The tests are passing for Python 2 but I'm thinking we should stop supporting Python 2. I've just created #401 to discuss this. We probably want to make those changes before landing this PR.
| if bye: | ||
| raise exceptions.IMAPClientAbortError(bye[-1].decode(self._encoding, 'replace')) | ||
|
|
||
| def check_state(self, *states): |
There was a problem hiding this comment.
This should probably have an underscore prefix. It shouldn't really get called externally.
| def _subcommand_and_check(self, meth, *args, **kwargs): | ||
| """Call another method and check the result code. | ||
|
|
||
| Return the data from the command. If unpack=True, return data[0]. | ||
| """ | ||
| unpack = pop_with_default(kwargs, 'unpack', False) | ||
| assert not kwargs, "unexpected keyword args: " + ', '.join(kwargs) | ||
|
|
||
| typ, data = meth(*args) | ||
| self._checkok(meth.__func__.__name__.upper(), typ, data) | ||
| if unpack: | ||
| return data[0] | ||
| return data |
There was a problem hiding this comment.
It seems to me that this is somewhat unnecessary. The methods passed to _subcommand_and_check all seem to use _simple_command so if _simple_command was extended to do the OK checking and optional unpacking we wouldn't need _subcommand_and_check at all.
I'm ok with this being a stepping stone and being removed in a later PR if you prefer.
| """ | ||
| self.check_state('AUTH') | ||
|
|
||
| name = 'PROXYAUTH' |
| #!/bin/bash | ||
|
|
||
| podman run --name=dovecot --rm -d -p 14301:143 dovecot/dovecot | ||
|
|
||
| cat >/tmp/livetest.ini <<EOF | ||
| [DEFAULT] | ||
| host = localhost | ||
| username = user | ||
| password = pass | ||
| port = 14301 | ||
| ssl = false | ||
| EOF | ||
|
|
||
| python livetest.py /tmp/livetest.ini | ||
|
|
||
| podman rm -f dovecot |
There was a problem hiding this comment.
This is great. I hadn't seen podman before.
|
|
||
|
|
||
| class IMAP4WithTimeout(imaplib.IMAP4): | ||
| class IMAP4WithTimeout(conn.IMAP4): |
There was a problem hiding this comment.
With this PR, there's no reason for this to exist any more. The timeout functionality should just be rolled into classes in conn.py.
2 participants
I felt like taking a stab at this. It seems to work, though I've only tested against Dovecot. I added a
livetest.shfile to automate this with podman.Basically, I incorporated all the calls from imaplib that imapclient was using into IMAPClient or a new conn.IMAP4 class for the low-level stuff. I divided
_command_and_checkinto 2 versions:_uid_command_and_check-> just calls_simple_command, optionally with UID prefix._subcommand_and_check-> wraps an old imaplib method. No UID support.It should be easy to do a bit more refactoring on a command-by-command basis, merging the old _cmd_XXX() call (that's from imaplib) into the imapclient parent method. Most of the work might actually be updating the unit tests.
There are a few more things to note. It seems like
ACCEPT=UTF8support was broken, sinceimaplib.IMAP4.enable()wasn't be called - that's where the magic happens. So I just removed that stuff for now. Also, there were a few calls imaplib supported that client doesn't - like get/set annotations. I left those in as private methods.I'm open to any feedback.