Developers of Sympa

[Guillaume Rousse <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
}

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

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

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

> 2- Soji proposed to name this sub "serialize" instead of "store" which
> may be more explicit;
Fine with me.

> > 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.
--
Guillaume Rousse
INRIA, Direction des systèmes d'information
Domaine de Voluceau
Rocquencourt - BP 105
78153 Le Chesnay
Tel: 01 39 63 58 31

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