Developers of Sympa

[IKEDA Soji <address@concealed>]

Hi,

On Thu, 19 Sep 2013 17:35:16 +0200
David Verdin <address@concealed> wrote:

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

I'm not confident of unusefulness of 'warn' level, while by now I
can also not prove usefulness of it.  So I'll withdraw it.

On 'debug' level, I don't worry about performance.  Even when logging
is disabled inside log_message() (perhaps because debugging level is
low enough), this function shall be called.  When it was enabled,
decrese of performance is the result of configuration done by
administrator.

Database logging looks to hit performance much for me.  It might
be disabled by a configuration parameter.

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

My concern on exception:

- It tends to skip clean-up processes inside "try" and other blocks.
  Not only skipping each logic, it is likly to cause leaks of
  rosources such as memory.  The function corresponding to "finally"
  may probably ease such situation, while it can worsen the next
  problem.

- It is a bit hard to read.  The idiom using eval {} and if () is
  obvious (additionally, by this idiom, programmers must re-throw
  uncaught exceptions by themselves).  Syntax sugers provided by
  several modules are not always acceptable by average Perl
  programmers.

I'm not an opponent of exception, contrary, I prefer to this
concept.  But by now I'm undecided.

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

I have no objection.

>   * 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)

Difference from PBP are:

-ce : Cuddled else (PBP : -nce)

-et=4 : Entab leading 4 whitespace (PBP : none)

-nsbl : No opening sub brace on new line (PBP : -sbl)

-wba : Add following operators
  || && . ? : and or xor


In above, "-et=4" seems to be inconsistent with your proposal.

Addition to -wba (by me) sometimes generates a bit misterious
results.


Regards,


--- Soji

-- 
株式会社 コンバージョン  セキュリティ&OSSソリューション部   池田荘児
〒231-0004　神奈川県横浜市中区元浜町3-21-2 ヘリオス関内ビル7F
e-mail address@concealed  TEL 045-640-3550
http://www.conversion.co.jp/