Issue 2550576
Created on 2009-08-18 03:40 by jerrykan, last changed 2011-01-18 10:07 by schlatterbeck.
Files | ||||
---|---|---|---|---|
File name | Uploaded | Description | Edit | Remove |
mailgw.py.patch | jerrykan, 2010-10-18 00:44 | |||
mailgw.py.newpatch | schlatterbeck, 2010-12-21 21:53 | new patch against original mailgw.py |
Messages | |||
---|---|---|---|
msg3852 | Author: [hidden] (jerrykan) | Date: 2009-08-18 03:40 | |
It would be nice if _handle_message in roundup/mailgw.py was split up into many functions instead of one large one. I imagine it would have benefits for maintainabilty, but... I have a custom mailgw script that inherits from MailGW, and I am currenly defining my own _handle_message to change parts of how emails are handled as they come in. For the most part it is identical to the original _handle_message, but is does contain a few small alterations. I am basically duplicating ~600 lines of code just the change a few small parts of it. If _handle_message was split up into many functions, it could override only the ones I want to change, without having to duplicate large chunks of code (and help improve compatability with future versions). |
|||
msg4154 | Author: [hidden] (jerrykan) | Date: 2010-10-18 00:44 | |
Quite a large patch to split the _handle_message function up into multiple smaller functions and decouple some of the code in the process. The evolution of this patch can be viewed at: http://gitorious.org/roundup/roundup/commits/mailgw |
|||
msg4212 | Author: [hidden] (schlatterbeck) | Date: 2010-12-21 21:53 | |
Hello John, I really like the idea of splitting up the mail parsing. I've taken a closer look at the patch. I like the way you semantically dissected the large method into smaller ones. There are some (small) errors so I've not (yet) accepted it as-is. The following corrections would be necessary: - _handle_message_get_pgp_message currently defines the function pgp_roles after using it some lines above there currently is no regression test for the pgp feature... I'm not using it myself. - the db.commit that commits a user is called two times, once in the method building the author _handle_message_get_author_id and once in the top-level _handle_message routine. The call in the _handle_message_get_author_id method is wrong, it commits before some other checks are done. - some values are recomputed several times. This may introduce hard-to-find bugs in the future (and *decreases* maintainability) because a change might fix one computation and miss another. One example is the recomputation of message.extract_content in two separate methods (and only using one of the two returned values) When refactoring this I'd introduce a new class, lets call it "parsedMessage". In this I'd put all the _handle_* methods as class methods of the new class (stripping the prefix _handle_message). Then the constructor of the new class would call all your _handle methods and build the data structures. Finally a method committing all this to the database would be called by the original _handle_message routine. Maybe the intermediate commit would also be taken out of the __init__. This would also avoid lots of passed arguments: Each of the new methods will introduce some object variables as self.<variable>, e.g., we would compute the class only once as self.cl (bad example, this *is* later explicitly recomputed when the database is reopened for the new user) Maybe we can even add this to the currently existing Message class. If you have time I'd appreciate if you again work on that patch. Alternatively I might look into this more closely but this might take more time.... I'd probably take the route of committing the fixed patch (attached) and working from there to factor the code into a separate class. I'm attaching a modified patch that fixes the first two issues above. Thanks for this work Ralf |
|||
msg4213 | Author: [hidden] (jerrykan) | Date: 2010-12-23 05:01 | |
Hello Ralf, Thanks for taking to time to look at this. It has been a while since I've looked at the code, so I'll need to reacquaint myself with it :D I am happy to have to have another look at the patch in the (hopefully) not too distant future. It is somewhat dependant on work commitments, but I'll let you know how I go. Cheers, John. |
|||
msg4214 | Author: [hidden] (schlatterbeck) | Date: 2010-12-23 15:46 | |
Thanks John -- I had some time now and used your code to factor the _handle_message code into a separate class. This avoids recomputing values and in particular the parameter-passing. Otherwise this is quite identical to your refactoring. I had to fix the bugs mentioned earlier, added some more tests for things that were not covered (message id generation) and have also fixed the parameter parsing (-C and -S options) to work with other classes not named "issue" (hopefully, for this there is no test yet). Fixed in r4577 |
|||
msg4215 | Author: [hidden] (jerrykan) | Date: 2010-12-24 01:14 | |
Hello Ralf, I was hoping to have another look at the patch and provide some additional feedback about the patch before anything was committed (unfortunately holiday session has already kicked in though). I have some reservations about about the 're-factoring into a separate class'. The initial aim of this patch (for me at least) was so that it would make it easier to alter parts of the MailGW for specific cases user may have. A simple example might be that I don't want to send a help message back to the user. Using my initial patch I could achieve this by having my own script that would be something like: # import roundup stuff here class OurMailGW(MailGW): def _handle_message_help(self, message): return # define 'instance' and 'mailbox' here gw = OurMailGW(instance, {}) gw.do_mailbox(mailbox) But with the approach of using a separate class, I would need to: * create my own parsedMessage class (OurParsedMessage) * override the 'handle_help' function in OurParsedMessage * create my own MailGW class (OurMailGW) * override the '_handle_message' and duplicate the entire contents just so I can change the following line: parsed_message = OurParsedMessage(self, message) This second approach seems a lot less maintainable, and less intuitive. The size of the MailGW class may be reduced a bit by moving the code out of it, but I'm not sure it introduces some significant drawbacks. |
|||
msg4221 | Author: [hidden] (schlatterbeck) | Date: 2011-01-10 16:07 | |
John, First a good new year to you. Thanks for the feedback. You are raising a valid point here, one of the goals of the refactoring is easier override of the message parsing and that isn't achieved with my change. I still think that it's a good idea to put all the state during message parsing into a separate object for maintainability reasons. I've now introduced in r4578 a class variable of MailGW named parsed_message_class that by default points to the parsedMessage in mailgw.py, you still have to override both classes but the code will not duplicate existing long methods. With that change your use-case becomes: # import roundup stuff here class OurParsedMessage(parsedMessage): def handle_help(self): return class OurMailGW(MailGW): parsed_message_class = OurParsedMessage # define 'instance' and 'mailbox' here gw = OurMailGW(instance, {}) gw.do_mailbox(mailbox) Can you live with that? Ralf |
|||
msg4222 | Author: [hidden] (jerrykan) | Date: 2011-01-11 02:06 | |
Hello Ralf, I haven't had a chance to play with the new changes, but at a quick glance it seems to be a good solution and one I am happy with (it would only require a few minor changes to my existing script). If I happen to notice anything else down the track, I can always create a new issue. So I would consider this issue "done" and thanks for helping to get the changes committed. Cheers, John. |
|||
msg4227 | Author: [hidden] (schlatterbeck) | Date: 2011-01-18 10:07 | |
On Tue, Jan 11, 2011 at 02:06:38AM +0000, John Kristensen wrote: > > John Kristensen <John.Kristensen@dpipwe.tas.gov.au> added the comment: > > Hello Ralf, > > I haven't had a chance to play with the new changes, but at a quick > glance it seems to be a good solution and one I am happy with (it would > only require a few minor changes to my existing script). If I happen to > notice anything else down the track, I can always create a new issue. So > I would consider this issue "done" and thanks for helping to get the > changes committed. Thanks for the feedback, let me know if you find something to fix when you have time to look into it. Ralf |
History | |||
---|---|---|---|
Date | User | Action | Args |
2011-01-18 10:07:57 | schlatterbeck | set | messages: + msg4227 |
2011-01-11 02:06:38 | jerrykan | set | messages: + msg4222 |
2011-01-10 16:07:47 | schlatterbeck | set | messages: + msg4221 |
2010-12-24 01:14:12 | jerrykan | set | messages: + msg4215 |
2010-12-23 15:46:27 | schlatterbeck | set | status: new -> closed assignee: schlatterbeck resolution: accepted messages: + msg4214 |
2010-12-23 05:01:37 | jerrykan | set | messages: + msg4213 |
2010-12-21 21:53:05 | schlatterbeck | set | files:
+ mailgw.py.newpatch nosy: + schlatterbeck messages: + msg4212 |
2010-10-18 00:44:27 | jerrykan | set | files:
+ mailgw.py.patch keywords: + patch messages: + msg4154 |
2009-08-18 03:40:11 | jerrykan | create |