Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Corriger les warnings relevés par brakeman plutôt que de les ignorer #4245

Merged
merged 8 commits into from
Apr 29, 2024

Conversation

francois-ferrandis
Copy link
Contributor

Le fichier brakeman.ignore contient une liste de warnings brakeman que nous avons observés et décidé d'ignorer.

Je trouve que ce fichier :

  • crée du bruit inutile car difficile à parser par un humain (nouveau dev, ou ancien dev curieux)
  • installe une culture de "on ignore les warnings plutôt que de les corriger"

Je corrige donc ici les warnings relevés et je supprime brakeman.ignore.

Checklist

Avant la revue :

  • Nettoyer les commits pour faciliter la relecture
  • Supprimer les éventuels logs de test et le code mort

Revue :

  • Relecture du code
  • Test sur la review app / en local

@francois-ferrandis francois-ferrandis self-assigned this Apr 25, 2024
Comment on lines -135 to +136
brakeman (5.3.1)
brakeman (6.1.2)
racc
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

J'ai lancé brakeman avant et après la MAJ, et il n'y a pas de nouvelles erreurs. 😉

Comment on lines -21 to +25
mail.from %("#{domain.name}" <#{default_from}>) if mail.from.blank?
mail.from(rfc5322_name_and_email(domain.name, default_from)) if mail.from.blank?
end

def rfc5322_name_and_email(name, email)
%("#{name}" <#{email}>)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ce code est testé, on peut le voir en cherchant " < dans les specs. 😉

= render @motifs
= render partial: "admin/motifs/motif", collection: @motifs, as: :motif
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Voir https://brakemanscanner.org/docs/warning_types/dynamic_render_paths/

Ça nous coûte pas cher d'utiliser cette version explicite, c'est plus lisible à mes yeux et ça rassure brakeman.

Copy link
Contributor

@victormours victormours left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trop bien
(et c'est des petits trucs, mais remplacer page par page_number ça fait partie de ces gains de lisibilité qui font vraiment plaisir)

@francois-ferrandis francois-ferrandis enabled auto-merge (squash) April 29, 2024 08:52
@francois-ferrandis francois-ferrandis merged commit 9ff8e47 into production Apr 29, 2024
11 checks passed
@francois-ferrandis francois-ferrandis deleted the frf/reduce-brakeman-noise branch April 29, 2024 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants