Proposition: Une version modifiée de la règle 'member-ordering'

[Julien Lamarre]

Un truc qui m'achale dans nos règles tslint présentes, c'est "member-ordering".

Nous utilisons présentement la valeur qui provient de tslint recommended: "statics-first". Voici la définition complète de la règle.

Ce qui m'achale principalement dans cette règle, c'est ce qui vient après le "constructor", c'est à dire l'ordre requis des méthodes, selon leur visibilité.

Il m'arrive souvent d'écrire une méthode public, puis de la refactoriser en plusieures méthodes plus petites ne devant pas, elles, être public... Je dois à ce moment ajouter ces méthodes non public à la toute fin de la classe, pour que la validation tslint passe! Je préfèrerais parfois être de mesure de regrouper certaines méthodes, quand cela fait du sens. Que ces méthodes soient public, protected ou private. Ceci résulterait en un code qui, à mon avis, serait souvent plus facile à suivre, analyser et modifier.

Je ne suggère pas de désactiver la règle en entier, mais de la tweaker. Pour permettre un ordre au choix pour ce qui est des méthodes.... Personnellement , je ferais la même même chose au niveau des fields, mais c'est moins important à mon avis.

Si on change cette règle, on dérive évidemment des règles suggérées par défaut. Mais si tous sont d'accord, je pense que ça pourrait améliorer notre code. Qu'en pensez-vous?

[Pierre VOISIN]

Pareil, ça m'achale régulièrement... En plus, dans l'historique Git on voit un gros pavé de changements non justifié à mon goût dès lors qu'une méthode change de visibilité. Pouvoir au peuple ! :D

[Stéphane Leblanc]

Je suis entièrement d'accord!

1) le coût du changement n'est pas significatif, car on enlève une contrainte. Les projets actuels n'ont pas à être modifiés.

2) plusieurs (dont moi) trouvent que cette règle n'apporte pas vraiment de valeur

3) en pratique, on perd quelques minutes quotidiennement à respecter cette règle

Je propose qu’on désactive complètement la règle, car il est parfois souhaitable qu’un field se retrouve près de son getter/setter.

[Martin ROY]

J'ai pas beaucoup développé cette dernière année, mais lorsque j'ai eu à le faire c'était plus souvent qu'autrement pour du refactoring et comme vous le dites, ça été un "pain in the ass" de devoir systématiquement gérer, organiser l'ordonnancement méthodes, en plus d'avoir un différenciel illisible dans git, donc deviens difficile de faire du code review, abbattons cette règle ;-)

[Yann DEBONNEL]

je pense que l'avantage du code mieux 'ordonné' est faible face aux désavantages énumérés, je suis d'accord avec vous

[Jérémie Carpentier-Roy]

Je suis d'accord avec cette proposition.

[Pierre VOISIN]

Il y a peut-être une combinaison (pour la configuration de la règle, cf. "order") qui serait raisonnable : cf. https://palantir.github.io/tslint/rules/member-ordering/

[Daniel Brodeur]

Même remarque que Pierre Voisin:

• ca nous demande de le placer à la toute fin à la suite de d'autre méthodes n'ayant pas nécessairement d'affinité avec
• ca contamine le diff laissant possiblement le reviewer dans un état de panique soudain
• perte de temps pour le developpeur avec un retour sur investissement qui est difficile à quantifier

Donc ce que je comprends c'est qu'on retirerait les valeurs suivantes du "statics-first":

 "statics-first",
        [
            "public-static-field",
            "public-static-method",
            "protected-static-field",
            "protected-static-method",
            "private-static-field",
            "private-static-method",
            "public-instance-field",
            "protected-instance-field",
            "private-instance-field",
            "constructor",
//            "public-instance-method",
//            "protected-instance-method",
//            "private-instance-method",
        ],

[Daniel Brodeur]

De dire qu'on désactive complètement la règle n'est pas souhaitable selon moi.

Avoir les éléments statiques au début de la classe est quelque chose qui est souhaitable à garder.

[Julien Lamarre]

PR: https://bitbucket.org/villemontreal/lint-config-villemontreal/pull-requests/33/tweaking-the-member-ordering-rule/diff

[Stéphane Leblanc]

Le pr avec la config ci-dessous est accepté. Merci à tous pour avoir participé à la discussion!

{
    'member-ordering': [
      true,
      {
        order: ['static-field', 'static-method', 'instance-field', 'constructor', 'instance-method']
      }
    ]
  }

[Julien Lamarre]

Je vais pousser sur Nexus demain...