Developers of Sympa

[David Verdin <address@concealed>]

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

address@concealed">Le 22/07/2013 13:53, David Verdin a écrit :

Hi Guillaume,


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

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.

A method is just a function in disguise, receving an object as implicit first parameter. You can achieve exactly the same result with both constructs.

package Sympa::Message;

sub my_method {
    my ($self) = @_;
    // do whatever you want with $self, an instance of Sympa::Message
}

package Sympa::Tools;

sub my_function {
    my ($message) = @_;
    // do whatever you want with $message, an instance of Sympa::Message
}

True. I just found it was more intuitive by using an object, but I'm open to alternatives.

address@concealed">

Using this class allowed me to simply call "BounceMessage::process" in
the bounced.pl main loop. It's far more readable.

Indeed, but you're mixing logic here:
- how to analyse a message content to identify who sent it
- what to do once the message has been analysed

Conceptually, the two rfc1819() and anabounce methods() (low-level primitives from first category) belong to Sympa::Message class, whereas delete_bouncer() and update_subscriber_bounce_history() (high-level code from 2nd category) belong to another piece of code. That's not a message's business to handle list subscription, or user history, that's application business.

OO-modeling does not means "everything related to XYZ goes to XYZ.pm file", it's rather 'everything which belongs to XYZ entity responsability goes into XYZ class".

Rightissime. I see clearly now the distinction you want to make: discern message parsing from the processing of the information contained by the message.

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?

Right. But in order to identify it as a bounce, you had to instanciate it as a normal Sympa::Message object, no :) ?

Heh! ;)
Actually, I use it in bounced.pl because I expect to handle bounces and nothing else. Actually, we could improve BounceMessage::new to return undef if the message is not a bounce. But then again, You made a point by discerning message analysis from user management.

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.

- 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.

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.

Indeed, serialisation method should just output content, and not deal with filenames.

Great! We agreed!

address@concealed">

2- Soji proposed to name this sub "serialize" instead of "store" which
may be more explicit;

Fine with me.

Two times!

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?).

We have a modeling issue here.

Basically, we have at least 3 piece of code related to parsing message content, in order to identify interesting parts:
 - virus_infected, in Sympa::Tools (200 lines)
 - rfc1819 (renamed parse_rfc1891_notification), in Sympa::Tools::Bounce (40 lines)
 - anabounce (renamed parse_any_notification), in Sympa::Tools::Bounce (400 lines)

For me, all of them should be treated similairly. Meaning either:
- as Sympa::Message methods,
- as methods from a specific subclasses (not much gain IMHO)
- as function taking a Sympa::Message object as argument.

Other methods from current BounceMessage should not be part of Sympa::Message namespace, in any manner. The same way we have a Sympa::Bulk class, to handle bulk executable logic, we should probably have a Sympa::Bounce class, to handle logic for bounced.pl logic.

Agreed. The point here is to know how to draw the line. Just see below the kind of reasonning that led me to create the BounceMessage object
All these Message / Bounce / task / etc. Classes have the common point to be found in spools - either as files or as DB entry.
that's their common point. We could then have a base object that would provide the informations necessary for spool management (list, queries, sorting, and so on). Consequently, The Spool object would just have to manipulate  collections of these objects.

Then, some of the objects in spool have more in common. list messages, bounces and messages to archive, for example are all RFC 822 valid messages and can then be processed as it.
Bounces have then some particularities : they contain codes that need to be analyzed to check if they are errors or return receipt, etc. This is a particular task and I didn't see it being inserted in a common Message object. That's where we differ. You consider that there is no added value to putting it in a dedicated message. I thought that, facotrizing-wise, it was more readable to put it apart from the rest.

Finally, as I knew we were handling bounces to increase the error note of users, I added user management function to the object because one you have a fully grown BounceMessage object, you laready know all you need to handle error score : which list, which user, which error. I focused on Sympa main usage of the bounces.

That's probably where I have difficulties drawing the line between functionnality and good refactoring.

Maybe we should put error manamgenet in User instead? There is also the question of MDN and DSN. They are both bounces, structurally speaking, but are not used to handle error, just to track message delivery.

Actually, the more I write about it, the more I'm confused. what would you say if I proposed a little visioconference around the end of the week?

We could speak about :
- the result of the merge,
- our current design concerns,
- our aims for Sympa 7.0, functionally, and maybe have some ideas about a calendar?

Regards,

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