search

fiuba / alfred

GNU General Public License v3.0
3 stars 8 forks source link

Feature disabling notification for test result #63

Closed diegosanchez closed 11 years ago

anero commented 11 years ago

Creo que salvo el comentario que puse (y que es solamente una cuestión de estilo así que podés obviarlo si no estás de acuerdo), el PR está para mergear.

diegosanchez commented 11 years ago

Me parece válido (es mucho más elegante). Ahí lo modifico y veo que todos los tests pasen.

2013/8/28 Diego Marcet notifications@github.com

Creo que salvo el comentario que puse (y que es solamente una cuestión de estilo así que podés obviarlo si no estás de acuerdo), el PR está para mergear.

— Reply to this email directly or view it on GitHubhttps://github.com/fiuba/alfred/pull/63#issuecomment-23429416 .

Saludos DiegoS

ar.linkedin.com/in/ingdiegosanchez

nicopaez commented 11 years ago

@diegosanchez entiendo que en un punto es una cuestión de gustos, pero la convención que he visto en general al usar feature toggling es hablar en términos de prender/apagar (on/off) funcionalidades, con lo cual apuntaria a algo del estilo send_result_notification? (true/false). @anero ¿que opinas?

nicopaez commented 11 years ago

Hago el merge, pero no me cierra como está implementado. Más alla del tema del PREVENT, creo que tener una clase para manejar el toggling del mail no cierra. Me parece que agregar una clase por solamente 1 cosa que hay que preder/apagar es over-design.