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

implement await/async, search, sort, thread, list-status, esearch, esort #52

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

filiphanes
Copy link

@filiphanes filiphanes commented Jul 24, 2020

  • fix FetchCommand.wait_data()
  • don't request capability when server sends it
  • support multiple untagged response types for LIST-STATUS and FETCH VANISHED
  • add response parsers
  • update README.rst

I have tested it only against Dovecot 2.4.devel

Fixes #47
Fixes #51
Fixes #45
Fixes #6

- fix FetchCommand.wait_data()
- don't request capability when server sends it
- support multiple untagged response types for LIST-STATUS and FETCH VANISHED
- add response parsers
- update README.rst
@hudcap
Copy link

hudcap commented Nov 10, 2020

Is there any update on this?
I'm getting

    with (yield from self.state_condition):
TypeError: 'Condition' object is not iterable

unless I change all references to self.state_condition to use the async/await keywords.

I didn't try too hard to debug, since this PR should fix it.

Copy link
Contributor

@darkrain42 darkrain42 left a comment

Choose a reason for hiding this comment

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

I still get odd test failures, but with the suggested changes, it gets further than:

Traceback (most recent call last):
  File "/home/paul/src/aioimaplib/aioimaplib/tests/imapserver.py", line 172, in execute_section
    with (await self.state_condition):
TypeError: object Condition can't be used in 'await' expression

aioimaplib/tests/imapserver.py Outdated Show resolved Hide resolved
aioimaplib/tests/imapserver.py Outdated Show resolved Hide resolved
@TheBlusky
Copy link

Is there any plan on implementing this ?

@bamthomas
Copy link
Owner

bamthomas commented Apr 9, 2021

Thank you for your feedbacks, addings and thoughts.

But I'm sorry, I cannot merge this PR :

  • tests are broken (and not one but a lot I'm not even sure that the library is still working properly)
  • there are duplicated code (like quoted/unquoted)
  • commented code
  • constants removed
  • and untested code

@Lithimlin
Copy link

Just letting people know that I'm also getting a similar error which I believe to be related:

File ".../env/lib/python3.9/site-packages/aioimaplib/aioimaplib.py", line 618, in wait
    with (yield from self.state_condition):
TypeError: 'Condition' object is not iterable

@Lithimlin
Copy link

But I'm sorry, I cannot merge this PR :
[...]
* there are duplicated code (like quoted/unquoted)
* commented code
* constants removed

Not sure what you mean by this. When I'm looking through the diff of the PR, I see no such things and while one constant's name has been removed, it was the CR constant which was only used once in the code.

Otherwise, this PR look pretty solid

@filiphanes
Copy link
Author

@bamthomas is right,

  • tests are broken, because I have tested it only against Dovecot server from my other project https://github.com/filiphanes/jmap-proxy-python/ which is using all of new code
  • where do you see duplicated code in quoted/unquoted?
  • I will remove commented code
  • constant CRLF replaced in 2 places to avoid resolving global variable and IMHO b'\r\n' is better readable and obvious
  • IMAP4_PORT and IMAP4_SSL_PORT were used only for default argument values, which again is more obvious to put plain values there, but I can revert it, if @bamthomas want
  • yes, tests should be fixed

@bamthomas
Copy link
Owner

what I mean @Lithimlin is pretty clear, there is no opinion or whatever here, only facts :

  • tests are broken : look above : do you see one commit with a valid green tick ? no
  • quoted/unquoted methods are duplicated functions with only a small diff before it wasn't the case
  • there is commented code because it was annoying
  • even if there was only one instance of CRLR it was more clear than \r\n and showed intention why remove this ?
  • and all the bunch of utility functions (that are I am sure are useful) but code without tests are debt (Michael Feathers)

so when all of this will be fixed then I could eventually merge.

@Lithimlin
Copy link

Lithimlin commented Apr 9, 2021

Sorry if there was a misunderstanding. I never questioned the tests failing, only the points I quoted. The tests are obviously failing.

I still don't see any duplicated or commented-out code. Or do you mean documentation comments in the code?It seems @filiphanes is also confused about this part, and I also agree with them regarding the CRLF constant.

Copy link
Owner

@bamthomas bamthomas left a comment

Choose a reason for hiding this comment

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

I won't take your copyright.
Either every ten committers are here or either none.
My name is here because I started this and still maintaining it.

@@ -1,6 +1,7 @@
# -*- coding: utf-8 -*-
# aioimaplib : an IMAPrev4 lib using python asyncio
# Copyright (C) 2016 Bruno Thomas
# Copyright (C) 2020 Filip Hanes
Copy link
Owner

Choose a reason for hiding this comment

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

:)
do you know that there has been 10 commiters and you are the first to do this ?

Copy link
Author

Choose a reason for hiding this comment

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

no :) I thought that it is normal when someone adds significant portion of functionality. Are there some limiting consequences for you?

@bamthomas
Copy link
Owner

I pulled the branch and yes the tests are green. But I can't merge this. I'm sorry, even if I think that there may be value in it.
there are too much mixed things and added functionalities without any tests.

So for now I'm blocked with this PR.
I can propose you to have a jitsi meeting to talk about this if you want. You can come with @Lithimlin if you want, and I can also join @pblayo that is the second committer on the lib. What do you think ?

@Lithimlin
Copy link

Since I have no real connection to either of you and am just eagerly waiting for an update on this not functional library I'd rather stay out of the discussion, though I appreciatethe invitation.
I only looked over the PR since I wanted to see what was changed. I find the changes personally to look very good and be codedina good way,however, I have not yet had the opportunity to test any of the changes.

@filiphanes
Copy link
Author

@bamthomas ok, I understand, I am willing to work on it, and would appreciate some help with tests

Meeting would be great, I would like to discuss these updates, you can send me email.

@bamthomas
Copy link
Owner

see the #59 is what we would expect.

@bamthomas
Copy link
Owner

@filiphanes I tried the form on your website but it says that the mail server is unreachable.
You can send me an email to bruno on the domain barreverte.fr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants