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

Make MailMover an ABC and have FolderMailMover as a subclass #147

Closed
wants to merge 1 commit into from

Conversation

kyrias
Copy link
Contributor

@kyrias kyrias commented Jun 10, 2017

This is as a preparation for implementing a version of #88 as an option. It makes MailMover an Abstract Base Class which means that anything deriving from it has to implement the proper methods to be able to instantiate them at all.

For an idea of how the end result will work, this PR adds a configuration option called global.mail_mover_kind which defaults to folder, which is the current MailMover behaviour. The follow-up PR with the functionality from #88 will add query as an option to global.mail_mover_kind which will read rules from the QueryMailMover configuration section and apply them as in #88.

Signed-off-by: Johannes Löthberg <johannes@kyriasis.com>
@kyrias
Copy link
Contributor Author

kyrias commented Jun 18, 2017

ping?

@GuillaumeSeren
Copy link
Collaborator

Hello @kyrias
Thank you for the work on the mail mover it is a very important feature to me !

I have checked the patch, to me it seems ok, but it would be better with a test don't you agree ?

Copy link
Collaborator

@GuillaumeSeren GuillaumeSeren left a comment

Choose a reason for hiding this comment

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

Add unit test

@flokli flokli added this to the 1.2.0 milestone Jun 19, 2017
@flokli
Copy link
Member

flokli commented Jun 19, 2017

I also really like this, however, I'm a bit unconfident to merge this as long as we don't have at least some tests for afew's main functions.

@GuillaumeSeren
Copy link
Collaborator

Hey @kyrias ,
it would e neat to get this merged, first it need to be rebased, then adding a test,
I understand testing such a case is not easy, maybe someone can help write the test.

@GuillaumeSeren
Copy link
Collaborator

Hey,
should we close this PR ? (I think yes)

Because owner is not active and MailMover is a big feature for afew,
we don't want to break it.

@flokli
Copy link
Member

flokli commented Dec 3, 2017

Well, as written before, with a proper test suite we don't have to be afraid of merging this. I'd keep this open until we have one.

@GuillaumeSeren
Copy link
Collaborator

Yes @flokli I was expecting this from you so I did not close it but if the owner will not upgrade the PR we finally need to create a new one with the tests

@@ -25,19 +25,19 @@
from .Database import Database
from .utils import get_message_summary
from datetime import date, datetime, timedelta
from abc import ABCMeta, abstractmethod
Copy link
Member

Choose a reason for hiding this comment

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

please sort import alphabetically

@@ -47,7 +47,47 @@ def __init__(self, max_age=0, rename = False, dry_run=False):
self.dry_run = dry_run
self.rename = rename

def get_new_name(self, fname, destination):
@abstractmethod
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the base class (ignoring the abstract here) provide a generic moving implementation?
Moving mails is the tricky part in this feature. We shouldn't encourage duplicating that effort.

@GuillaumeSeren
Copy link
Collaborator

Hello, @kyrias
As this PR is not active anymore, I am closing it.
Feel free to reopen if needed with upgrades/replys.

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

Successfully merging this pull request may close these issues.

None yet

4 participants