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

Fix 887 and improve code coverage of R/config.R from 28.21% to 100% #1043

Merged
merged 11 commits into from
Aug 8, 2023

Conversation

ilyaZar
Copy link
Contributor

@ilyaZar ilyaZar commented May 24, 2023

Fix #887

Problem:
Whenever golem::document_and_reload() calls get_golem_wd() to retrieve values from a yml-config, and the user changed the location of that yml before that call, golem::document_and_reload() is not aware of that location change and fails. Specifically, guess_where_config() cannot find the location when it is not of standard type (e.g. "inst/golem-config.yml").

Fix:
This PR solves this by adding a helper to guess_where_config() which finds the user config-yaml by reading its new location from user changes in "R/app_config.R".

@ALanguillaume
Copy link
Member

ALanguillaume commented May 26, 2023

Thanks a lot @ilyaZar !

I will take the time to review your PR asap. In the mean time I quickly eye-balled on the code and made a few comments.

@ilyaZar
Copy link
Contributor Author

ilyaZar commented May 27, 2023

Done:

Just refactored the logic for the whole file because it got complicated and messy. Here is a summary of all changes of this PR so far:

  • 31213b0: add a helper named try_user_config_location() inside guess_where_config() which finds a user supplied config-yaml by reading its new path location from changes in the "R/app_config.R" file
  • 83a96a7: fix a minor style issue - use && instead of & with logical scalars
  • 230b73d: add tests for this behavior checking that the config-yaml can be:
    1. renamed and
    2. moved to another location
  • 8e98808: improve readability and logic of try_user_config_location(); also add support for multi-line statements of app_sys() inside R/app_config.R; update tests to check the new behaviour improving code coverage of R/config.R to 63.16%
  • 90ee1ca: sub-function should output same object (fs-path type) to top level function to make comparison-checks easier; and fix a typo
  • a941ec1: rework guess_where_config() -> since the user can now set paths to his/her golem-configs this must be properly handled
  • a941ec1: add proper tests to check the previous behavior and improve code coverage for R/config.R to 68.87%
  • 095bfc5: add more tests to cover "exotic" corner cases when trying to find the golem-config; moves code coverage of the file to 70.75%
  • 3917328: remove redundant if-statement which is roughly a duplicate of the one before; worst case is that guess_where_config() fails slightly faster which is probably good as we can then handle the failure as another corner case explicitly
  • f3e6cb2: encapsulate yesno() into separate function call to improve function logic and make guess_where_config() testable in interactive mode via the package {mockery}
  • caf682a: based on the previous, add the mocking tests to obtain a coverage of 100% for R/config.R

image

  • 6529474: document the implemented features

To-Do:

  • inform the user with a print statement which config is used, or add a helper that prints the current location of the config in use?

- add helper try_user_config_location()
    - search for new config location to be set inside R/app_config.R
- update guess_where_config() with a final guess using this helper
- run styler::style_file("R/config.R", transformers =
  grkstyle::grk_style_transformers())
- path changes mean new dir and new filename so:
  - create a new dir for config
  - move config to that dir and rename

=> test correctly identifies changes (changing dir AND changing filename) in the yaml-config.

attribute to @ALanguillaume with some minor tweaks

Co-authored-by: ALanguillaume <[email protected]>
@ilyaZar
Copy link
Contributor Author

ilyaZar commented Aug 2, 2023

@VincentGuyader do you think above CI failures relate to #1072 or #1070 ? my forked branch passed the tests on my local machine hence the question.

if you point me somewhere I'll try to fix the PR myself to make it pass the CI. thanks!

…n()`

- encapsulate finding the relevant lines in the config file into "guess_lines_to_config_file()"
    - function allows for multi-line/grk-style app_sys()-calls (if file path to long and must be put into next line)
    - has clearer and commented logic to find the lines
- document proper logic of function with comments
- style with `grkstyle::grk_style_transformer`
- update `tests/testthat/test-config.R` to incorporate above changes and move code coverage of that file to 63.16%
- allows valid comparison with fs_path()'s inside guess_where_config() and output values from try_user_config_location()
- fix a typo in a comment
Why:

- clearly outline the different cases: since now the user can add a golem-config, there can be conflicting cases between the default golem config and the user golem config (if e.g. the user forgets to delete the default)
- update the tests to check these different cases
- remark: test inside testthat should work without golem:: prefixes
- test that config file can be found if
   - we end up inside "inst/" (a corner case not tested before)
   - guess_where_config() is fed with argument values for 'path' and 'file' that do not work

-> improves code coverage to 70.75%
- remove last if-statement (which is almost a duplicate of the one before)
   -> this makes the guess_where_config() fail easier (i.e. return NULL) as other exotic corner cases should be handled more explicitly
- remark: golem::pkg_path() will (almost) never produce an error but this may change so the try-construct within attempt() is kept

- improve comments for corner case IV.B as internal developer note
@ilyaZar ilyaZar changed the title Fix 887 Fix 887 and improve code coverage of R/config.R from 28.21% to 100% Aug 3, 2023
- notably: add package mockery to Suggests (no dependencies except for testthat and used for tests only)

- add tests for non-interactive usage of guess_where_config(): check that error is thrown correctly

- add tests for interactive usage of guess_where_config():
    - user says yes: test that necessary files are created
    - user says no: test that we return NULL

- add tests for encapsulated ask_golem_creation_upon_config()
    - shallow test as non-interactive menu() calls (inside the yesno()) are forbidden
    - but this makes code coverage 100% :)
- only exported function from R/config.R needs proper docs
- besides more details on older usage and behavior:
    - add docs on how to set user supplied golem config
    - add code snippets
@VincentGuyader VincentGuyader changed the base branch from dev to dev_887 August 8, 2023 09:23
@VincentGuyader VincentGuyader merged commit ba26195 into ThinkR-open:dev_887 Aug 8, 2023
0 of 5 checks passed
VincentGuyader added a commit that referenced this pull request Aug 8, 2023
* Fix 887 and improve code coverage of `R/config.R` from  28.21% to 100% (#1043)

* fix: add user supplied path to config for guess_where_config()

- add helper try_user_config_location()
    - search for new config location to be set inside R/app_config.R
- update guess_where_config() with a final guess using this helper
- run styler::style_file("R/config.R", transformers =
  grkstyle::grk_style_transformers())

* fix: logical scalar operation should use && instead of &

* test: guess_where_config() works with path changes

- path changes mean new dir and new filename so:
  - create a new dir for config
  - move config to that dir and rename

=> test correctly identifies changes (changing dir AND changing filename) in the yaml-config.

attribute to @ALanguillaume with some minor tweaks

Co-authored-by: ALanguillaume <[email protected]>

* refactor: improve readability and logical of `try_user_config_location()`

- encapsulate finding the relevant lines in the config file into "guess_lines_to_config_file()"
    - function allows for multi-line/grk-style app_sys()-calls (if file path to long and must be put into next line)
    - has clearer and commented logic to find the lines
- document proper logic of function with comments
- style with `grkstyle::grk_style_transformer`
- update `tests/testthat/test-config.R` to incorporate above changes and move code coverage of that file to 63.16%

* style: try_user_config_location() returns fs-paths for consistency

- allows valid comparison with fs_path()'s inside guess_where_config() and output values from try_user_config_location()
- fix a typo in a comment

* refactor: improve readability and logic inside guess_where_config

Why:

- clearly outline the different cases: since now the user can add a golem-config, there can be conflicting cases between the default golem config and the user golem config (if e.g. the user forgets to delete the default)
- update the tests to check these different cases
- remark: test inside testthat should work without golem:: prefixes

* test: add test for exotic corner cases of guess_where_config()

- test that config file can be found if
   - we end up inside "inst/" (a corner case not tested before)
   - guess_where_config() is fed with argument values for 'path' and 'file' that do not work

-> improves code coverage to 70.75%

* feature: guess_where_config() fails a bit faster

- remove last if-statement (which is almost a duplicate of the one before)
   -> this makes the guess_where_config() fail easier (i.e. return NULL) as other exotic corner cases should be handled more explicitly
- remark: golem::pkg_path() will (almost) never produce an error but this may change so the try-construct within attempt() is kept

- improve comments for corner case IV.B as internal developer note

* refactor: encapsulate yesno() for clarity and easier testing

* test: move coverage of R/config.R to 100%

- notably: add package mockery to Suggests (no dependencies except for testthat and used for tests only)

- add tests for non-interactive usage of guess_where_config(): check that error is thrown correctly

- add tests for interactive usage of guess_where_config():
    - user says yes: test that necessary files are created
    - user says no: test that we return NULL

- add tests for encapsulated ask_golem_creation_upon_config()
    - shallow test as non-interactive menu() calls (inside the yesno()) are forbidden
    - but this makes code coverage 100% :)

* docs: update docs for get_current_config()

- only exported function from R/config.R needs proper docs
- besides more details on older usage and behavior:
    - add docs on how to set user supplied golem config
    - add code snippets

---------

Co-authored-by: ALanguillaume <[email protected]>

* chore update news and Rd files

---------

Co-authored-by: Ilya Zarubin <[email protected]>
Co-authored-by: ALanguillaume <[email protected]>
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.

3 participants