Developers of Sympa

[David Verdin <address@concealed>]

Hi Guillaume,

Le 22/07/13 13:25, Guillaume Rousse a écrit :

address@concealed">Hello.

I'm discovering merge results.

I'm quite sceptic about the Sympa::BounceMessage usefulness, which basically turns all functions previously in Tools::Bounce to methods, attached to a new subclass of Sympa::Message.

Well, it allows to use these function with an object. The code is far lighter this way; Also, we don't need to bother about context: every data we need is in the object.
Also, we could use this inheritance to use the primitives Soji talked about to parse spools.
Using this class allowed me to simply call "BounceMessage::process" in the bounced.pl main loop. It's far more readable.

address@concealed">It means than if I have an instance of Sympa::Message, I first need to rebless it as a Sympa::BounceMessage first before being able to analyse it: bad design.

But if you analyze it, that means that, one way or another, you know you're handling a bounce; So why not instantiate it as a Bounce Message from the start?

address@concealed">
Morevoer, I'm also skeptic about methods such as store_bounce(), than just serialize the message content into some directory retrieved from configuration:
- how is serializing a bounce message different from serializing a normal message ?

Very very very little. You're right.

address@concealed">- it creates yet another dependency between a component (a message object) and the configuration, the kind of relationship I've been trying to kill for 6 months...

Yes, I apologize, but please see below.

address@concealed">
You can achieve exactly the same purpose by calling a yet-to-define store() methods in Sympa::Message class, and handling the target directory in the calling code.

Completely agreed. That's what we pay for merging 6.2 (where refactoring regarding spools was not finished yet) with your branch.
Two remarks:
1- the store_bounce was used to keep a copy of the last bounce received, not to store it in spool. The file it is stored in as then a different name that we would use. Consequently if we define a "store" method in Message.pm, we should have a way to have some control over the file name we will store the message, either a filename argument or a "get_destination_file()" method that could be overriden.
2- Soji proposed to name this sub "serialize" instead of "store" which may be more explicit;

address@concealed">
Basically, I'd either:
- move all methods of Sympa::BounceMessage directly in Sympa::Message
- keep all of them as functions in Sympa::Tools::Bounce

The later option has my preference, as we have exactly the same kind of problem with antivirus scanning.

I would say none of them: for the reason exposed above, we could move Sympa::BounceMessage to Sympa::Message::Bounce, and move anything not specific to bounce handling to  Sympa::Message (or Sympa::Message::Base?).

Anyway, about this topic, I think we should let Soji do what he had in mnd regarding spools and objects handled by spools. He put a hold on it to let me mùerge the two branches but he looked like ha had a lot of ideas about it.
Why not let it as it is right now and wait to see what Soji will do once the merge is over?

address@concealed">
BTW, there is no 'use Sympa::BounceMessage' anywhere in the code currently...

But there is one "use BounceMessage" in bouced;pl.in. Don't forget we didn't move alle the code to the "Sympa" name space in 6.2 branch.

One last thing: I'm not done with the merge yet, so please make sure you work on new modules only (those that currently pollute the "src" namespace.
the files and directories listed in the temporary file "merge-conflict" at the root of the sources are those for which I still need to do the merge.

Thanks for going back in the saddle so quickly!

David

--
A bug in Sympa? Quick! To the bug tracker!

David Verdin Infrastructure pour les Services Informatiques
Tél : +33 2 23 23 69 71 Fax : +33 2 23 23 71 21   www.renater.fr	RENATER 263 Avenue du Gal Leclerc 35042 Rennes Cedex

Attachment: smime.p7s
Description: Signature cryptographique S/MIME