RLesur commented 4 years ago

close #1

RLesur commented 4 years ago

@MaelTheuliere wrote in https://github.com/spyrales/gouvdown/issues/1#issuecomment-621649317:

Au passage, j'ai un soucis avec gouvdown-imports.Rd qui me bloque pour l'utilisation du pipe dans les fonctions. J'ai l'habitude d'utiliser usethis::use_pipe() mais là évidemment les deux sont en conflits. Une idée ? Sachant que j'ai pas trouvé où était créé ce gouvdown-imports.Rd

D'après ce que je comprends, usethis::use_pipe() a déjà été utilisé dans cette PR.

Mais au-delà de ça, a-t-on vraiment besoin d'importer et d'exporter le pipe à ce stade du développement. Ca rajoute une dépendance à magrittr qui ne me semble pas nécessaire :thinking:

tvroylandt commented 4 years ago

De ce que je vois, il y a seulement deux pipes dans les fichiers non ? Donc on peut enlever.

Question un peu plus large : est-ce si "grave" d'avoir une dépendance à magrittr qui est bien maintenu et relativement light ?

MaelTheuliere commented 4 years ago

ça me va je les enlève

RLesur commented 4 years ago

Question un peu plus large : est-ce si "grave" d'avoir une dépendance à magrittr qui est bien maintenu et relativement light ?

De façon générale, je trouve que c'est bien de chercher à limiter les dépendances. Le CRAN a d'ailleurs prévu de limiter les dépendances à 20 maximum, il me semble.

Dans le cadre du développement d'un package, je vois assez peu d'intérêt à utiliser le pipe. Il me semble qu'un pipe c'est plus d'une quinzaine d'appels de fonctions imbriqués alors qu'on peut faire ça avec une simple assignation (un seul appel de fonction). Je trouve qu'utiliser le pipe dans le code source d'un package fait un peu "usine à gaz" pour un intérêt plus que limité.

En revanche, si l'API que le package offre est compatible avec le pipe, là oui, je comprends tout à fait qu'on l'importe pour le réexporter : c'est d'ailleurs ce que j'essaie de faire au maximum. Les utilisateurs finaux aiment bien le pipe (et moi aussi).

Je dois reconnaître que j'ai pris l'habitude de coder très différemment lorsque je développe un package ou lorsque je fais un script "utilisateur final" : je n'ai pas de problème à utiliser des dépendances pour un script de traitement de données mais je réfléchis toujours beaucoup avant d'en rajouter une à un package. Je suis peut-être biaisé.

tvroylandt commented 4 years ago

Question un peu plus large : est-ce si "grave" d'avoir une dépendance à magrittr qui est bien maintenu et relativement light ?

De façon générale, je trouve que c'est bien de chercher à limiter les dépendances. Le CRAN a d'ailleurs prévu de limiter les dépendances à 20 maximum, il me semble.

Dans le cadre du développement d'un package, je vois assez peu d'intérêt à utiliser le pipe. Il me semble qu'un pipe c'est plus d'une quinzaine d'appels de fonctions imbriqués alors qu'on peut faire ça avec une simple assignation (un seul appel de fonction). Je trouve qu'utiliser le pipe dans le code source d'un package fait un peu "usine à gaz" pour un intérêt plus que limité.

En revanche, si l'API que le package offre est compatible avec le pipe, là oui, je comprends tout à fait qu'on l'importe pour le réexporter : c'est d'ailleurs ce que j'essaie de faire au maximum. Les utilisateurs finaux aiment bien le pipe (et moi aussi).

Je dois reconnaître que j'ai pris l'habitude de coder très différemment lorsque je développe un package ou lorsque je fais un script "utilisateur final" : je n'ai pas de problème à utiliser des dépendances pour un script de traitement de données mais je réfléchis toujours beaucoup avant d'en rajouter une à un package. Je suis peut-être biaisé.

C'est une excellente réponse ! Merci.

codecov[bot] commented 4 years ago

Codecov Report

Merging #15 into master will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #15   +/-   ##
=======================================
  Coverage   53.36%   53.36%           
=======================================
  Files           6        6           
  Lines         208      208           
=======================================
  Hits          111      111           
  Misses         97       97

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a2ee3bf...a2ee3bf. Read the comment docs.

MaelTheuliere commented 4 years ago

@RLesur je ne peux pas te mettre en reviewer (car tu as ouvert la pr j'imagine ?). Mais ton avis est le bienvenue ;-)

MaelTheuliere commented 4 years ago

corrigé, a mettre dans un test en effet.

Le mar. 5 mai 2020 à 15:03, Romain LESUR notifications@github.com a écrit :

@RLesur commented on this pull request.

In R/bloque_marque.R https://github.com/spyrales/gouvdown/pull/15#discussion_r420090851:

+#' @param bloque_marque name of the bloque marque

+#'

+#'

+#' @return a string

+#' @export

+

+get_bloque_marque <- function(bloque_marque) {

+

liste_bloque_marque <- list_bloque_marque()

if (!bloque_marque %in% liste_bloque_marque) {

stop("Error Message: this bloque_marque is not know")

}

dir <- pkg_resource("bloque_marque",bloque_marque)

files <- list.files(dir,full.names = TRUE)

return(files)

En principe elle doit renvoyer un seul fichier par construction des répertoires.

Je suis un peu perdu : get_logo("gouvernement") renvoie 2 fichiers. En effet, il y en a deux dans inst/resources/blocs_marque/gouvernement https://github.com/spyrales/gouvdown/tree/95395fc61a32cb51ab0177eb890dfd723ac07687/inst/resources/blocs_marque/gouvernement .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/spyrales/gouvdown/pull/15#discussion_r420090851, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAQE2HAHE5NORQSEFR6AEX3RQAFBHANCNFSM4MVICS3A .

RLesur commented 4 years ago

J'ai voulu rajouter des exemples dans la doc hier soir et ça m'a amené à me poser des dizaines de petites questions sur l'API offerte par les fonctions, le choix des noms de variables, le choix des noms de fonctions, etc.

Par exemple, les noms des fonctions get_logo(), create_logo() et add_logo() sont très proches et elles font des choses très différentes. A l'utilisation, j'étais un peu perdu. Je me disais que logo_file_path(), gglogo() et add_plot_header() me parleraient beaucoup plus.

Plutôt que de vous embêter avec mes milliers de problèmes existentiels, je me suis permis de commiter directement dans la branche en modifiant pas mal de petites choses. J'ai cherché à simplifier et éclaircir certains points et faire un petit peu de ménage. J'espère que vous ne m'en voulez pas trop. Je ne voulais pas trop ralentir le process.

Il y avait aussi une note qui restait au moment du check (à cause des warnings). Bref, j'ai passé un coup de polish final avec de merger la branche.

J'ai aussi failli commencer à rajouter des tests mais je me suis souvenu que @MaelTheuliere avait dit qu'il le ferait. Donc, je me suis arrêté...

MaelTheuliere commented 4 years ago

J'ai voulu rajouter des exemples dans la doc hier soir et ça m'a amené à me poser des dizaines de petites questions sur l'API offerte par les fonctions, le choix des noms de variables, le choix des noms de fonctions, etc.

Par exemple, les noms des fonctions get_logo(), create_logo() et add_logo() sont très proches et elles font des choses très différentes. A l'utilisation, j'étais un peu perdu. Je me disais que logo_file_path(), gglogo() et add_plot_header() me parleraient beaucoup plus.

Plutôt que de vous embêter avec mes milliers de problèmes existentiels, je me suis permis de commiter directement dans la branche en modifiant pas mal de petites choses. J'ai cherché à simplifier et éclaircir certains points et faire un petit peu de ménage. J'espère que vous ne m'en voulez pas trop. Je ne voulais pas trop ralentir le process.

Il y avait aussi une note qui restait au moment du check (à cause des warnings). Bref, j'ai passé un coup de polish final avec de merger la branche.

J'ai aussi failli commencer à rajouter des tests mais je me suis souvenu que @MaelTheuliere avait dit qu'il le ferait. Donc, je me suis arrêté...

C'est très biren comme ça @RLesur ! C'est vrai que les noms des fonctions sont plus claires comme cela.

MaelTheuliere commented 4 years ago

J'ai voulu rajouter des exemples dans la doc hier soir et ça m'a amené à me poser des dizaines de petites questions sur l'API offerte par les fonctions, le choix des noms de variables, le choix des noms de fonctions, etc.

Par exemple, les noms des fonctions get_logo(), create_logo() et add_logo() sont très proches et elles font des choses très différentes. A l'utilisation, j'étais un peu perdu. Je me disais que logo_file_path(), gglogo() et add_plot_header() me parleraient beaucoup plus.

Plutôt que de vous embêter avec mes milliers de problèmes existentiels, je me suis permis de commiter directement dans la branche en modifiant pas mal de petites choses. J'ai cherché à simplifier et éclaircir certains points et faire un petit peu de ménage. J'espère que vous ne m'en voulez pas trop. Je ne voulais pas trop ralentir le process.

Il y avait aussi une note qui restait au moment du check (à cause des warnings). Bref, j'ai passé un coup de polish final avec de merger la branche.

J'ai aussi failli commencer à rajouter des tests mais je me suis souvenu que @MaelTheuliere avait dit qu'il le ferait. Donc, je me suis arrêté...

Sur les test je peux rajouter ceux ci ce soir dans le pr comme ça ce sera fait. Je vois les test suivants :

vérifier que gglogo() fonctionne pour tous les logos présents
logo_file_path() renvoi un argument et un seul argument pour tous les répertoires
vérifier que theme_gouv() est un theme d'autres idées ?

MaelTheuliere commented 4 years ago

Il semble que la nouvelle version de ggpubr en date du 5 mai casse l'installation. ça me fait penser qu'on pourrait switcher sur patchwork pour le même usage. Arguments :

même mainteneur que ggplot2
moins de dépendances
focalisé sur cet usage d'assemblage de plot

si vous êtes ok, je peux fixer

MaelTheuliere commented 4 years ago

Il semble que la nouvelle version de ggpubr en date du 5 mai casse l'installation. ça me fait penser qu'on pourrait switcher sur patchwork pour le même usage. Arguments :
* même mainteneur que ggplot2

* moins de dépendances

* focalisé sur cet usage d'assemblage de plot
si vous êtes ok, je peux fixer

J'ai regardé, c'est plus chiant, patchwork ne travaillant que sur des objets ggplo2

RLesur commented 4 years ago

Il semble que la nouvelle version de ggpubr en date du 5 mai casse l'installation. ça me fait penser qu'on pourrait switcher sur patchwork pour le même usage. Arguments :
* même mainteneur que ggplot2

* moins de dépendances

* focalisé sur cet usage d'assemblage de plot
si vous êtes ok, je peux fixer
J'ai regardé, c'est plus chiant, patchwork ne travaillant que sur des objets ggplo2

Le problème ne vient pas de là. Je vais réparer.

MaelTheuliere commented 4 years ago

Il semble que la nouvelle version de ggpubr en date du 5 mai casse l'installation. ça me fait penser qu'on pourrait switcher sur patchwork pour le même usage. Arguments :
* même mainteneur que ggplot2

* moins de dépendances

* focalisé sur cet usage d'assemblage de plot
si vous êtes ok, je peux fixer
J'ai regardé, c'est plus chiant, patchwork ne travaillant que sur des objets ggplo2
Le problème ne vient pas de là. Je vais réparer.

super merci @RLesur. Pour moi on est bon donc ?

spyrales / gouvdown

Thèmes ggplot2 #15

Codecov Report

@RLesur commented on this pull request.