avis sur un script

Le Mon, 02 Mar 2009 18:02:54 +0000, Alex écrivait:

- les constantes sont généralement écrites tout en majuscules, donc
pattern pourrait s'appeler PATTERN (ou quelque chose de plus significatif)

Corrigé, même si je garde PATTERN (rien de mieux me vient à l'esprit)

- les variables globales sont généralement à éviter (cf. import this).
Pour un petit script en ligne de commande, ce n'est pas vraiment un
problème, mais si ton code se retrouve un jour appelé depuis une appli
plus volumineuse, tu peux rencontrer des conflits de nommage et des
problèmes d'accès concurrents. En pratique, on a quasiment jamais besoin
de variables globales.

Tout à fait d'accord pour les globales.
Je vais regarder un peu plus de code python mais comment dé-globaliser
(oh le vilain mot :) des variables comme debug, verbose ? A part en
rajoutant des paramètres aux fonctions mais je crains que cela
n'alourdisse de code. Mais je vais investiguer :)

- le module getopt n'est utilisé nulle part (import inutile),

Oups, corrigé.

félicitations pour avoir pensé à utiliser OptionParser !

On me l'a soufflé à l'oreille mais je trouve que ce module est une
tuerie. J'ai relu la doc plusieurs fois pour être sûr de ne pas passer à
côté d'une élégante subtilité.

Merci pour toutes ces remarques.

Le script mis à jour est disponible ici:
http://download.bsdsx.fr/ftppurge.py

--
freddy <point> dsx <arobase> free <point> fr

> Je vais regarder un peu plus de code python mais comment dé-globaliser
(oh le vilain mot :) des variables comme debug, verbose ? A part en
rajoutant des paramètres aux fonctions mais je crains que cela
n'alourdisse de code. Mais je vais investiguer :)

Le plus souvent, il faut penser à définir une classe qui contient
exactement les informations dont la fonction à besoin. Comme ça, si à un
moment tu remarques que tu as besoin d'un paramètre en plus, tu ajoute
une méthode à la classe (avec self.enPlus = ValeurParDefaut dans __init__).

Bonne soirée
Laurent

Alex Marandon a écrit :

- le module getopt n'est utilisé nulle part (import inutile),
félicitations pour avoir pensé à utiliser OptionParser !

Juste pour ma culture personnelle : qu'est-il reproché à /getopt/ ?

--
22ème jour sans liaison..

Freddy DISSAUX a écrit :

Le Mon, 02 Mar 2009 18:02:54 +0000, Alex écrivait:

- les constantes sont généralement écrites tout en majuscules, donc
pattern pourrait s'appeler PATTERN (ou quelque chose de plus significatif)

Corrigé, même si je garde PATTERN (rien de mieux me vient à l'esprit)

La question à se poser dans ce genre de cas, c'est ce qu'est censé
identifier ta regexp. Ici, ce sont les noms des fichiers à supprimer...

Accessoirement, tu pourrais aussi précompiler la regexp:

FILENAMES_EXP = re.compile(r'.*-d{8}.tbz')

après quoi l'appel se fait:

if FILENAMES_EXP.match(filename):
# code here

- les variables globales sont généralement à éviter (cf. import this).
Pour un petit script en ligne de commande, ce n'est pas vraiment un
problème, mais si ton code se retrouve un jour appelé depuis une appli
plus volumineuse, tu peux rencontrer des conflits de nommage

<Alex>
Dans la mesure où les "globales" de Python ne sont globales qu'au
module, le seul risque de conflit de nommage serait avec un import *,
qui est une mauvaise pratique dans l'écrasante majorité des cas.
</Alex>

et des
problèmes d'accès concurrents.

Là par contre je plussois.

En pratique, on a quasiment jamais besoin
de variables globales.

<mode="pédant">
Rappel : def et class sont des instructions exécutables, qui instancient
un objet resp. function ou class et le lie au nom dans l'espace de
nommage en cours. Donc les classes et fonctions définies au top-level
d'un module sont techniquement des variables 'globales' (au moins dans
le sens Python de 'global')...
</mode>

Ceci étant, dans le cas présent, je plussois également !-)

Tout à fait d'accord pour les globales.
Je vais regarder un peu plus de code python mais comment dé-globaliser
(oh le vilain mot :) des variables comme debug, verbose ?

L'objet 'option' retourné par OptionParser.parse_arg permet déjà
d'éviter d'avoir à passer plusieurs variables distinctes. Donc une
solution simple est de passer ce seul objet aux fonctions concernées.

Après, il y a la solution de créer une classe "DirectoryProcessor", et
de faire de process_dir et process_file des méthodes de cette classe.

Le script mis à jour est disponible ici:
http://download.bsdsx.fr/ftppurge.py

Quelques remarques encore:

def process_dir(f, dir):

le nommage est assez malheureux AMHA. 'f' n'est pas explicite (il faut
lire l'appel de la fonction pour comprendre ce qu'est 'f'), et 'dir'
masque la fonction builtin de même nom.

try:
f.cwd(dir)
except ftplib.all_errors, e:
print e

rappel : la sortie standard est destinée aux sorties *normales* du
programme. Les erreurs, warnings, et verboseries diverses ont un flux
spécifique, sys.stderr.

f.cwd('/')
return

personnellement, je laisserai l'appelant gérer l'exception - si tu veux
utiliser le code dans d'autres condition (GUI etc).

try:
f.cwd(dir)
except ftplib.all_errors:
f.cwd('/')
raise

try:
files = f.nlst()
except ftplib.error_perm, resp:
if str(resp) == "550 No files found":
if debug: print "%s: no files in this directory" % dir
else:
raise

Deux erreurs de logique ici AMHA:

1/ dans un cas, tu relance l'erreur (ce qui interrompt la fonction),
dans l'autre tu affiches une info *et* tu continue l'execution. Au vu du
reste du code, je ne suis pas sûr que ce soit vraiment ce que tu veux faire.

2/ accessoirement, dans deux autres cas, tu reviens à la racine du
répertoire ftp, mais pas là. Est-ce cohérent ?


for file in files:

Ceci masque le builtin 'file' - et n'est pas très exact puisque c'est un
nom de fichier, pas un objet fichier.

if re.match(PATTERN, file):



process_file(f, file)

f.cwd('/')

def process_file(f, file):

Même remarques que plus haut sur le nommage

items = re.split("[-.]", file)

Pourquoi cette expression est-elle définie localement alors qu'une autre
est définie globalement ?

if keep_first:
if items[-2][-2:] == '01':

pas très explicite.

if debug: print "Skip %s (keep_first)" % file
return
date_file = int(items[-2])

idem. Un peu de documentation ne ferait pas de mal ici (tu sera content
de la trouver quand tu devra debugger ou faire évoluer ce script dans 6
mois ou un an).

if date_limit > date_file:
if verbose:
print "Delete %s (%d > %d)" % (file, date_limit, date_file)
if not debug:
try:
f.delete(file)
except ftplib.all_errors, e:
print e

Ton option "debug" sert deux choses distinctes:
- dans le reste du code, elle est plus ou moins synonyme de 'verbose'
- à cette endroit précis, elle agit comme un dry-run

Il serait peut-être préférable de mieux distinguer ces deux rôles ?


Mes deux centimes...

Freddy DISSAUX, le 02/03/2009 a écrit :

[...]

Tout à fait d'accord pour les globales.
Je vais regarder un peu plus de code python mais comment dé-globaliser
(oh le vilain mot :) des variables comme debug, verbose ? A part en
rajoutant des paramètres aux fonctions mais je crains que cela
n'alourdisse de code. Mais je vais investiguer :)

Vous fabriquez un dictionnaire d'options et le passez en premier
paramètre à chaque fonction. Ou à l'__init__ de la classe si vous avez
préféré l'objet. Ce n'est absolument pas coûteux même si le
dictionnaire est velu puisqu'on passe la référence au dictionnaire. En
fait vous passez volontairement trop à chaque fonction.

C'est facile à faire, un truc du genre:

optionsdic = oppars.parse_args()[0].__dict__

"oppars", c'est pour vous "parser". Dans mon tout premier programme en
python - donc c'est sans doute pourri - , j'avais ça:

options = oppars.parse_args()[0]
optionsdic = dict((x, y) for x, y in options.__dict__.items() if y)

ce qui doit être légèrement différent pour les paramètres non
renseignés sans valeur par défaut. Mais je fais maintenant en sorte
d'avoir toujours une valeur, ou None.

On ajoute ce qu'on veut à optionsdic. On peut aussi remplacer
l'affectation par un update(). Je pense qu'on peut faire alors
simplement:

optionsdic.update(oppars.parse_args()[0])

à la place de:

optionsdic.update(oppars.parse_args()[0].__dict__)

Ainsi vous avez la partie utile de votre module, le main() qui vous
permet de l'utiliser en ligne de commandes, et la possibilité de
l'importer et de l'utiliser à partir d'un autre programme qui peut être
une interface graphique. Il vous suffit de fabriquer optionsdic.

--
Pierre Maurette

Freddy DISSAUX wrote:

Tout à fait d'accord pour les globales.
Je vais regarder un peu plus de code python mais comment dé-globaliser
(oh le vilain mot :) des variables comme debug, verbose ? A part en
rajoutant des paramètres aux fonctions mais je crains que cela
n'alourdisse de code. Mais je vais investiguer :)

En l'occurence, la ligne commençant par 'global' est-elle nécessaire ?
Ça ne marche pas avec juste des variables de module, c'est-à-dire
simplement avec les affectations que tu fais déjà en début de script ?

Le script mis à jour est disponible ici:
http://download.bsdsx.fr/ftppurge.py

Alex Marandon a écrit :

Freddy DISSAUX wrote:

Tout à fait d'accord pour les globales.
Je vais regarder un peu plus de code python mais comment dé-globaliser
(oh le vilain mot :) des variables comme debug, verbose ? A part en
rajoutant des paramètres aux fonctions mais je crains que cela
n'alourdisse de code. Mais je vais investiguer :)

En l'occurence, la ligne commençant par 'global' est-elle nécessaire ?
Ça ne marche pas avec juste des variables de module, c'est-à-dire
simplement avec les affectations que tu fais déjà en début de script ?

Non, il les mets à jour dans main() à partir des options retournées par
optparse.

Le Tue, 03 Mar 2009 12:33:49 +0100, Bruno écrivait:

[ snip globale ]

L'objet 'option' retourné par OptionParser.parse_arg permet déjà
d'éviter d'avoir à passer plusieurs variables distinctes. Donc une
solution simple est de passer ce seul objet aux fonctions concernées.

Adopté

Quelques remarques encore:

Chouette !

[ snip nommage et sys.stderr ]
Adopté

[ mauvaise utilisation de try, except, raise ]
Corrigé

2/ accessoirement, dans deux autres cas, tu reviens à la racine du
répertoire ftp, mais pas là. Est-ce cohérent ?

Utilisation de finally

[ masquage de builtin ]
Corrigé, j'utilise filename maintenant

Ton option "debug" sert deux choses distinctes:
- dans le reste du code, elle est plus ou moins synonyme de 'verbose'
- à cette endroit précis, elle agit comme un dry-run

Il serait peut-être préférable de mieux distinguer ces deux rôles ?

Je pense avoir corrigé

Mes deux centimes...

Avec grand plaisir (à moins de transformer ces centimes en bière*S* :)

http://download.bsdsx.fr/ftppurge.py

Merci à tous pour toutes vos remarques particulièrement constructive.
--
freddy <point> dsx <arobase> free <point> fr

Freddy DISSAUX a écrit :

Le Tue, 03 Mar 2009 12:33:49 +0100, Bruno écrivait:

(snip)

Quelques remarques encore:

Chouette !

Puisque tu aimes ça, j'en ai encore une paire. Mais bon, là c'est
vraiment histoire de conserver ma réputation d'insupportable chieur
pédant et tout !-)

0/ opérations sur les dates:

"""
now = time.time() - options.nb_days * 24 * 3600
options.date_limit = int(time.strftime("%Y%m%d", time.localtime(now)))
"""

Tu devrais peut-être jeter un oeil sur le module datetime ?


1/ sur le nommage:

"filesname" => "filenames"
"day_file" => "file_day"
"date_file" => "file_date"

"options.file_test" => "options.filename_test" ?

Accessoirement (mais là c'est personnel), raccourcir "ftp_handle" en
"ftp" tout court - je pense que dans le contexte, c'est suffisant pour
qu'on comprenne.

2/ indentation:

Bien que ce soit "légalement" correct, la forme

if condition: expression

est considérée comme une mauvais pratique.

3/ factorisation:

Ton code est parsemé de "if verbose: print >> sys.stderr, message". Il
serait plus simple de factoriser ce pattern dans une fermeture
positionnée en attribut de l'objet options. Quoi ? C'est pas clair ?
Bon, ok, je te le refais en Python:

def main():
....
# au passage, les parenthèses sont inutiles et peu pythonesques
(options, args) = parser.parse_args()

# setup
def verbose_print(what):
if options.verbose:
print >> sys.stderr, what

options.verbose_print = verbose_print

....

# utilisation:

if options.debug:
options.verbose = True

options.verbose_print("Host: %s" % options.host)
# etc...

Mes deux centimes...

Avec grand plaisir (à moins de transformer ces centimes en bière*S* :)

Si tu passes sur Bordeaux, sans problème !-)

Le Wed, 04 Mar 2009 10:48:03 +0100, Bruno écrivait:

Puisque tu aimes ça, j'en ai encore une paire. Mais bon, là c'est
vraiment histoire de conserver ma réputation d'insupportable chieur
pédant et tout !-)

Ne pas confondre chieur pédant et expérimenté soucieux du détail, gage
de qualité

0/ opérations sur les dates:

[ snip ]

Tu devrais peut-être jeter un oeil sur le module datetime ?

Corrigé. Effectivement, on croit faire le malin en transformant des
chaines "YYYYMMDD" en nombre pour faire des comparaisons/calculs mais ce
n'est jamais une bonne idée.

1/ sur le nommage:

"filesname" => "filenames"
"day_file" => "file_day"
"date_file" => "file_date"

"options.file_test" => "options.filename_test" ?

C'est ce petit genre de détail qui fait que l'on a plaisir à relire du
code. Adopté.

Accessoirement (mais là c'est personnel), raccourcir "ftp_handle" en
"ftp" tout court - je pense que dans le contexte, c'est suffisant pour
qu'on comprenne.

A trop vouloir en faire, on risque de devenir illisible. Adopté.

2/ indentation:

Bien que ce soit "légalement" correct, la forme

if condition: expression

est considérée comme une mauvais pratique.

Alors je ne le ferais plus jamais. Promis :)

3/ factorisation:

Ton code est parsemé de "if verbose: print >> sys.stderr, message". Il
serait plus simple de factoriser ce pattern dans une fermeture
positionnée en attribut de l'objet options. Quoi ? C'est pas clair ?

Déjà vu ça en Perl, mais je n'utilise pas trop. C'est vrai que cette
profusion de "if verbose" rendais mon code moche. Et quand je dis moche,
je parle bien de la "gueule" du code. J'aime bien prendre un peu de
recul (mois d'un mètre quand même) et *regarder* le code.
C'est pourquoi j'apprécie de plus en plus python: les contraintes de
rédaction font qu'il est facile d'avoir un beau code source et donc
agréable à lire, donc plus facile à comprendre, donc plus facilement
maintenable.

Bon, ok, je te le refais en Python:

Miam :)

[snip fermeture positionnée en attribut de l'objet options ]

J'en ai donc déduit options.debug_print.

Si tu passes sur Bordeaux, sans problème !-)

Heureux homme, faire du python à Bordeaux :)

Le script est toujours disponible ici:

http://download.bsdsx.fr/ftppurge.py

Encore merci pour tout.
--
freddy <point> dsx <arobase> free <point> fr