Developers of Sympa

[David Verdin <address@concealed>]

Hi guys,

Technically speaking, the merge between 6.2 and sympa-cleanup branches is over.

Technically.
That means we are no longer doing anything with SVN, but still have to fix all the mess that is the by-product of such a complicated merge.
We apologize for the regressions you will find in some parts of the code. Sometimes, we had to make clear decision regarding the code only, not indentation. In such occasions, it is possible that the new naming conventions were not respected - our mistake.
Some modules are messed up, like List.pm in which the merge was desastrous. I'll review it thouroughly to fix it all.
Anyway, now, anybody is free to work on whichever part of the code located in the sympa-cleanup branch. We all work on this branch now, until we find a satisfactory result.

Now, I want to sum up our recent discussion and propose some final guidelines.

First of all, I want to say that I'm currently - finally, should I say - reading the excellent "Perl best practices" by Damian Conway and I agreed with almost everything until now. I helps me understanding some of the decisions taken by Guillaume since the beginning of his refactoring - and some of Soji's, too. I think you both read this book, didn't you?
I propose we stick to these recommendations, except if you want to make some exceptions. Please comment anything you don't agree, so that we can reach an agreement soon.
We will then publish receommendations for developpers, being clear that these are only suggestions - we won't enforce our coding practices to them, only for the four of us.

Logs

• We will get rid of interpolation in do_log calls (replace Sympa::Log::do_log('err', "Could not open $file"); by Sympa::Log::do_log('err', 'Could not open %s', $file);)
• We will inspect logs to distinguish, amongst the different "debug" levels, which are relevant to end users and which are not. The first will be put in log_message calls, the former in log_trace_message calls. There is a problem with this simplification: some functions are called very often and, if all debug logs are actually printed, in an instance with a big activity, the performances will severely decrease. It can even stall the server. So this simple dichotomy between developpers and users could be problematic.
• 'warn' log level is useless. Let's have only two levels: "info" and "err". That's enough.
• Sympa::Log::Database is aimed at providing web-readable logs for list owners. It logs only main events to make them available through Sympa web interface.
• the web specific logs are useful because they provide informations specific to the web. It is important when analyzing logs. All three logging functions have their rational. I don't know how to simplify this part of the code.
  

Exception handling seems not to be resolved. the fatal_err calls were usefull because they specified explicitely that Sympa should not keep on running under certain circumstances. This is a clear distinction with the return undef we used as exception handling.
I am under the impression that all return undef could be replaced by croak calls, providing we intercept exceptions at the right place and we use the Carp option allowing to produce stak traces.
But, we need to distinguish the very few circumstances in which sympa should stop running, so that the daemons will stop gracefully. Inspecting the code, it looks like all fatal_err calls were made in the main daemons, so I think it should be easy.

Lock

• We will replace use of flock(2) by NFS::Lock in Lock.pm, as suggested by Soji,

Spools

• We will enforce the filesystem spools for now, not the SQL spools, until we have a mechanism to allow both. The reason here is that we know file system spools work whereas SQL spools proved unsafe on our production environment.
  

Naming conventions

• Unification of accessors : It's a hard topic, so I'll try to sum it up.

• Soji originally rewrote Site/Robot/Family/list and added an AUTOLOAD function allowing to dynamically create function corresponding to configuration parameters,
• Guillaume objected to the number of the resulting functions, as well as the double usage of such function: getters and setters ; he argumented for more explicit naming scheme.
• A consensus could be :

• get_* and set_* for any field belonging to an object. All the fields should be private, being manipulated by these getters/setters,
• a "get_parameter" and a "set_parameter" function for each parameter coming from a configuration file

Soji objected that such an organization didn't tackle the question of readonly parameters. This might be simply resolved by a "readonly" flag in the parameter definition, making the program croak if a developper tries to change it. readonly flag would be handled by "set_parameter" function. I suggest that we try to have a consistent naming for functions and variable. Let's use underscore-separated strings, with no capitals.

Code formatting

• Let's indent with spaces. 4 spaces width.
• We will not use perltidy in a post-commit hook. it could mess up carefully hand-crafted code layout.
• We should however run perltydy once before continuing bug fixing, so that, if some of the layout is messed up, it is at least at a time when we have to read the code a lot and potentially fix some abusive line feeds.
• do everybody agrees with the following settings?
  

-bar   # Opening brace always on right (* no)
-bbt=1 # Medium block brace tightness
-bt=1  # Medium brace tightness (* 1)
-ce    # Cuddled else (* no)
-cti=0 # No extra indentation for closing brackets
-et=4  # Entab leading 4 whitespace (* none)
-i=4   # Indent level is 4 cols
-ci=4  # Continuation indent is 8 cols
-l=78  # Max line witdh is 78 cols
-nolq  # Don't outdent long quoted strings
-nsbl  # No opening sub brace on new line (* -sbl)
-nsfs  # No space before semicolons
-pt=1  # Medium parenthesis tightness (* 1)
-sbt=1 # Medium square bracket tightness (* 1)
-se    # Errors to STDERR
#-st   # Output to STDOUT
-vt=2  # Maximal vertical tightness
-wba="% + - * / x != == >= <= =~ !~ < > | & >= < = **= += *= &= <<= &&= -= /= |= >>= ||= .= %= ^= x= || && . ? : and or xor"
# Break after all operators (* not contains "||" and tokens appear after it)

Documentation

• We shall stick to the current practice of documenting using POD
• we will keep on putting sub documentation right above the subs (that's my only disagreement with Damian Conway until now). I think that improves code comprehension.
• The template for documenting source files is the following:

=head1 FUNCTIONS

=head2 foo($bar)

=head3 parameters

=over

=item * $bar

=back

=head3 Return value

Something

=cut

Using enumeration for function/method names would force us to use explicit depth level to each enumerations:

=head1 FUNCTIONS

=over

=item * foo($bar)

=over 2

=item + parameters

=over 4

=item - $bar

=back

=item + return value

=back

Database access

• Étienne and I want simple high level subs to do queries. That means that parts of the SDM framework should comme back: those where error handling, statement handler creation and such where created without the developper bothering wbout it. We would like even higher level subs that I had the time to develop:

• get_hash_from_query which has the text of a query as argument and return a hash containing the result
• get_first_query_entry : same as above, but return the first result only

        I know that you got rid of the previous subs to remove a dependency to send_notif_to_listmaster. However, I think it might be possible without this dependency, by throwing exceptions instead.

Short term refactorings

• Configuration unification : I propose we keep our current configuration primitives, which give readable configuration files. We will extend them to allow longer and deeper paragraphs, but I don't think YAML is really necessary. However, we must merge the different functions used to handle configuration
• We need to remove the unused parameters, as Soji suggested
• I need to read the exception handling part in Conway's book, but we should generalize them. I'm OK with croaking if we correctly intercept exceptions and it removes dependencies.
  

Longer term refactoring objectives

• No more "robot". Let's replace this word with the more consensual "vhost"

Well, taht's it. I think I went through most of our recent discussion and proposed a consensus. Here's to you now!

Cheers,

David

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