Fix 887 and improve code coverage of R/config.R from 28.21% to 100%
#1043
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. |
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:
To-Do:
|
- 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]>
|
@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
changed the title
R/config.R from 28.21% 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% :)
- 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
9 tasks
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #887
Problem:
Whenever
golem::document_and_reload()callsget_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".