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

feat: Inflate_all() #204

Merged
merged 43 commits into from Jun 8, 2023
Merged

feat: Inflate_all() #204

merged 43 commits into from Jun 8, 2023

Conversation

ymansiaux
Copy link
Contributor

@ymansiaux ymansiaux commented Apr 20, 2023

TODO:

  • Fix unit tests
    image

-> devtools::check() performed by @ymansiaux on 2023-05-11

All like:

Warning (test-inflate-part2.R:59): (code run outside of `test_that()`)
path[1]="/tmp/RtmpJoImwR/all.templates.inflate6b9525bdf9ae/minimal": Aucun fichier ou dossier de ce type
  • Test inflate_all_no_check() to verify it does not check ?
    done by @ymansiaux on 2023-05-10 : a39a1a5

  • Test what happens with "keep" section: test driven
    done by @ymansiaux on 2023-05-10 : 8e4901d

  • Edit inflate-params writing in config_fusen.yaml to deal appropriately with logicals.
    Logicals were written as yes/no, we want them to be stored as TRUE/FALSE
    done by @ymansiaux on 2023-05-11 : a253b4e

  • See if we can put in function the document & check part used similarly in ìnflate() and inflate_all()
    done by @ymansiaux on 2023-05-11 ba45a5a

  • See if there is still a need to separate test_that() calls for easier maintenance

  • Open issue for documentation refactor

  • Faire tourner inflate_all() et inflate_all_no_check() sur {fusen} lui-même pour voir. Gérer les messages d'erreurs plus explicites pour l'utilisateur
    image

issue #96
close #157

tags: feat, wip

Why?

- The inflate_all() function is tested

What?

- We need to finalize the documentation and split the current flat file into two flat files

Issues

issue #96
tags: doc

Why?

-

What?

-

Issues

issue #96
tags: doc

Why?

-

What?

-

Issues

issue #96
tags: fix

Why?

-

What?

-

Issues

issue #96
@ymansiaux ymansiaux marked this pull request as ready for review April 20, 2023 12:32
ymansiaux and others added 9 commits April 20, 2023 14:06
tags: fix, test

Why?

- df_to_config() shouldnt be allowed to receive inflate parameters when working with "keep"

What?

- Edited the function, doc, and unit tests

Issues

issue #96
tags: feat, test

Why?

warnings comes with the call of the function it was called, here it was not really informative

What?

- Add call. = FALSE to clean the warning output
tags: feat, doc, test

Why?

- Stop also outputs the call by default

What?

- Clean stop call output to avoid do.call(xxx) message
tags: fix

Stop the process if Cancel is checked

issue #157
tags: fix, test

Why?

- Showing messages with {cli} for clarity

What?

- Update unit test
- Add function to be tested for outputs only
- Refactor unit test structure with multiple test_that calls for easier maintenance
tags: feat

Why?

- Need to document and check once only

What?

- Add check an document as in inflate
tags: refactor, test

Why?

- Unit tests output is long and multiple messages are useless for debugging
- New version of R does not like to readlines with missing final line

What?

- Added some suppressMessages()
- Added extra line in `cat()` created files
tags: fix, test

Why?

-

What?

- qpdf had to be installed on workbench
- replaced some expect_equal by expect_true(all(...))

Issues

issue #96
@statnmap
Copy link
Member

statnmap commented May 5, 2023

https://tech.lgbt/@debruine/110315357001043525


list(x=TRUE, y=FALSE, z = "true", a = "a") |>
yaml::as.yaml(handlers = list(
 logical = function(x) {
 result <- ifelse(x, "true", "false")
 class(result) <- "verbatim"
 return(result)
 }
))

tags: ci

Why?

- Trying to resolve CI errors

What?

-

Issues

issue #96
tags: ci

Why?

-

What?

-

Issues

issue #96
tags: ci

Why?

-

What?

-

Issues

issue #96
tags: ci

Why?

-

What?

-

Issues

issue #96
tags: fix, feat, test, ci

Why?

- We had issues using devtools::check because of a missing MASS library

What?

- check() automatically builds a package before calling check_built(), as this is the recommended way to check packages. Note that this process runs in an independent R session, so nothing in your current workspace will affect the process. Under-the-hood, check() and check_built() rely on pkgbuild::build() and rcmdcheck::rcmdcheck().

Issues

issue #96
tags: fix

Why?

-

What?

-

Issues

issue #96
tags: test

Why?

-

What?

-

Issues

issue #96
tags: feat

Why?

-  Logicals were written as yes/no, we want them to be stored as TRUE/FALSE

What?

- https://tech.lgbt/@debruine/110315357001043525

Issues

issue #96
tags: feat

Why?

- The same operation was done in inflate() and inflate_all(), we needed a function

What?

- see document_and_check_pkg()

Issues

issue #96
@statnmap
Copy link
Member

Merci de continuer à avancer sur ce sujet !

Je vois que tu as retiré {devtools} pour mettre {rcmdcheck}.
Je comprend pourquoi tu as fait ça car j'ai déjà eu ce problème.
Pour simplifier le problème, je dirai que c'est une question de fonctionnement de {devtools} dans un environnement interactif ou non.

Je pense qu'il faut remettre {devtools} plutôt que {rmcdcheck} parce que les utilisateurs vont utiliser {fusen} dans un environnement interactif et non pas dans un contexte comme nos tests unitaires.
Donc il faut faire en sorte que {fusen} corresponde à ce qu'un utilisateur voudrait et non pas s'ajuste aux problèmes que l'on rencontre en tant que dev.

Je te propose de regarder ce qu'il se passe dans les tests "inflate-part-1". Tu verras que j'ai mis des inflate(check = FALSE) puis plus tard mis à rcmdcheck() dans le test unitaire, pour éviter de tomber sur les mêmes problèmes que tu as vu dans le CI.
Tu noteras que le check = TRUE je ne le teste que dans inflate-part-2" dans un environnement interactif.
Et c'est la raison pour laquelle "dev/dev_history" contient des lignes de code pour lancer les tests dans la console, et donc en interactif.

On pourrait discuter du vrai pourquoi. Pourquoi tu t'es senti obligé de déclarer MASS, et tous les packages r-base ?
C'est parce que le devtools::check() travaille dans un libPaths indépendant de façon à n'avoir accès qu'aux packages listés dans ton DESCRIPTION et donc pour s'assurer que tu as bien tout déclaré.
Cependant, ce libPaths temporaire et créé avec un lien symbolique vers le r-base.
Lorsque tu lances un devtools::check() à l'intérieur du devtools::check(), il fait de même. Mais il ne se rend pas compte que le lien symbolique vers le r-base est à conserver. Du coup, il le supprime pour avoir un environnement le plus simple possible. Mais comme il ne peut pas créer le lien symbolique vers r-base à partir du lien symbolique vers r-base du check() parent, alors il est perdu.
Cela ne remet pas en cause le fonctionnement de {fusen}. Ça n'a pas d'impact sur les utilisateurs. Même si les utilisateurs lancent {fusen} dans un environnement non interactif, le check fonctionnera comme attendu.
Ça n'a un impact que pour les dev de {fusen} (nous !) qui veulent check que le check fonctionne...
C'est la joie de créer des packages pour simplifier la vie des autres dev ! Les packages pour faire des packages, c'est compliqué.

tags: feat, test

Why?

- According to @statnmap message on 2023/05/11 (#204) we decided to revert the use of rcmdcheck.

What?

- inflate_all(check = TRUE) are now only tested in interactive mode

Issues

issue #96
tags: fix, doc, ci

Why?

-

What?

-

Issues

issue #96
@ymansiaux
Copy link
Contributor Author

Merci pour ta réponse c'était très instructif !

J'ai fait des modifications en conséquent. J'ai par contre des soucis avec la github action R-CMD-check-devel.yaml

Cf log https://github.com/ThinkR-open/fusen/actions/runs/5090267208/jobs/9148872503?pr=204

-> L'installation de {rcmdcheck} ne se fait pas (voir ligne 3067). La dépendance {curl} ne peut pas s'installer (voir à partir de la ligne 3539 pour + d'infos).

@ymansiaux
Copy link
Contributor Author

ymansiaux commented May 30, 2023

Il semblerait que certaines dépendances systèmes nécessaire pour curl soient manquantes :

image

On note un problème dans la récupération des dépendances systèmes dans le fichier github actions

image

En théorie Ubuntu 20.04 est censé être encore supporté :
https://github.com/rstudio/r-system-requirements#operating-systems

Peut etre ouvrir une issue dédiée ?

tags: chore, ci

- Use the latest version of actions to apply to 'devel' action (the one that use the github version of parsermd)
- Use ubuntu-devel
tags: fix, test

path_foosen does not exist till it is not created. normalizePath need mustWork=FALSE to avoid sending a warning.
tags: fix, test

- Add extra line in Rproj file.
tags: fix, test

- It was anticipated a problem with new version of attachment, but it is finally not the case. We can verify it as it is now on CRAN
tags: refactor, test, doc

- As clean phase is not ready, we keep it separated for later developments
tags: fix, test

- `force` parameter should allow to add files in the config file even if they do not exist
tags: doc

- Manage NEWS with unpublished 0.5.0
- Re-write inflate_all doc
tags: fix, test

- Modification of files needs to be done using name instead of position as 'sort' changes in non-interactive environments
- Fix 'sort' by 'order' in unit tests to properly select a row

issue #96
tags: chore

- Need to inflate a few flats before being able to use inflate_all()
- Or define state
tags: fix, test

- A dev version of fusen has no "state" in "keep" section

issue #96
tags: feat, doc

- Opening a vignette is an individual choice

issue #96
tags: fix

- check() outputs need to be printed in the console
tags: feat

There is no general use to see the path of the current package being inflated. It only matters for unit tests
@statnmap statnmap merged commit 43aa1b1 into main Jun 8, 2023
8 of 10 checks passed
@statnmap statnmap deleted the 96-inflate_all branch June 8, 2023 07:58
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.

bug: Cancel "Add {fusen} chunk"
2 participants