Address ropensci review #213

pat-s · 2020-01-07T22:22:03Z

fixes use_tic(): Ensure that a DESCRIPTION file is present #200

mention the difference between .com and .org -> new vignette "org-vs-com")
indeed, perhaps the default R_TRAVIS should be .com given the current changes to travis? -> changed in {travis}
During travis::travis_sync() the API token is pasted in the console and (although deleted form history) is still visible in the console. Could this be avoided in any way? -> done when refactoring {travis}
return early if do_* and step_* functions are called interactively
error and suggest to call use_tic() first when running dsl_init() in a repo without a tic.R
Make it possible to pass the endpoint arg from {travis} funs to use_tic()
README updates
all examples are not \dontrun because dsl_init() only works within a project

Because of

https://github.com/ropenscilabs/tic/blob/834b30f0c63b004015154bfb94fd8c30b18cb5c8/R/stage.R#L35-L45

I had to move {usethis} from suggests to imports. Otherwise {usethis} is missing as it is not yet installed in the "install" step as a dep when being called in tic::prepare_stages().

I would like to avoid this actually but we are using quite a bit of {usethis} internally all over the place and maybe we should just have it in imports.

krlmlr · 2020-01-12T19:31:31Z

I resolved conflicts and tentatively undid the code changes. We can discuss.

This is what a call to do_bookdown() looks like now:

tic::do_bookdown()
#> Using TRAVIS_DEPLOY_KEY env var as the private key name for SSH deployment.
#> ✓ Creating a blank tic stage configuration
#> ℹ tic configuration
#> ── install ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── stage ──
#> ▶ step_install_deps(repos = repo_default())
#> ── deploy ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── stage ──
#> ▶ step_build_bookdown()

^{Created on 2020-01-12 by the reprex package (v0.3.0)}

We can add a note that points to ?tic::use_tic here.

Testing for interactive code is brittle: users might want to run tic::run_all_stages() in their interactive session.

I also don't think that {tic} should unconditionally require a DESCRIPTION. Rather it's the responsibility of the steps to check that a file is available, and err appropriately. Perhaps in their "prepare" step? (The "troubleshooting" section needs to mention prepare_all_stages() I think.)

Perhaps we should link more prominently to the "troubleshooting" section, to assist first time users? Or move "troubleshooting: local runs" to the "Get started" document?

krlmlr · 2020-01-12T19:35:58Z

Regarding proj_get(): Again, I think tic can expect to find a tic.R and everything else in its working directory, at least on the CI system. What motivated the change here?

krlmlr · 2020-01-12T20:11:29Z

Currently:

tic::do_bookdown()
#> Using TRAVIS_DEPLOY_KEY env var as the private key name for SSH deployment.
#> ✓ Creating a clean tic stage configuration
#> ℹ See `?tic::dsl_get` for details
#> 
#> ── tic configuration ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
#> ── install ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── stage ──
#> ▶ step_install_deps(repos = repo_default())
#> ── deploy ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── stage ──
#> ▶ step_build_bookdown()

^{Created on 2020-01-12 by the reprex package (v0.3.0)}

The help page explains what's going on, pointing to the tic.R file and the vignette.

Checking DESCRIPTION in steps that need it. Added prepare_all_stages() to troubleshooting. Added one more link to troubleshooting.

krlmlr · 2020-01-12T20:12:00Z

@pat-s: I can't request your review. Could you please take a look?

pat-s · 2020-01-13T09:59:08Z

Regarding proj_get(): Again, I think tic can expect to find a tic.R and everything else in its working directory, at least on the CI system. What motivated the change here?

The motivation came from the ropensci review where Anna tried calling some tic functions on a new repo without a tic.R file - i.e. no run of use_tic() as the first action.

This is what a call to do_bookdown() looks like now:

Should we expand the message something like: "We detected you are running XYZ interactively, which is not supported. See ?tic::dsl_get for details."?

I think we have room to improve the documentation on how to use {tic} interactively even though we have https://docs.ropensci.org/tic/articles/advanced.html#emulating-a-ci-run-locally already:

mention to not call steps/macros standalone
use tic::run_stage("<stage>") or tic::run_all_stages()

This would have helped Anna in her review I assume. We should also prominently link such a resource in interactive calls to macros/steps like the one above. What do you think?

pat-s · 2020-01-13T09:59:54Z

R/local.R

@@ -4,17 +4,22 @@ LocalCI <- R6Class(

  public = list(
    get_branch = function() {
-      system2("git", "rev-parse --abbrev-ref HEAD", stdout = TRUE)
+      suppressWarnings(system2("git", "rev-parse --abbrev-ref HEAD", stdout = TRUE))


Can we add a comment what we are suppressing here?

pat-s · 2020-01-13T10:00:21Z

R/local.R

+      tryCatch(
+        {
+          remote <- gh::gh_tree_remote()
+          paste0(remote$username, "/", remote$repo)
+        },
+        error = ""
+      )


What motivated this tryCatch?

pat-s · 2020-01-13T10:00:37Z

R/local.R

@@ -23,7 +28,7 @@ LocalCI <- R6Class(
      NULL
    },
    get_commit = function() {
-      git2r::revparse_single(revision = "HEAD")$sha
+      tryCatch(git2r::revparse_single(revision = "HEAD")$sha, error = "")


What motivated this tryCatch?

pat-s · 2020-01-13T10:03:12Z

R/steps-install.R

@@ -9,6 +9,9 @@ InstallDeps <- R6Class(
    },

    prepare = function() {
+      if (!file.exists("DESCRIPTION")) {
+        stopc("No DESCRIPTION file found. The step_install_deps step and the do_package_checks macro are only available for packages.")


I recently like to format error messages as

cli_alert_danger("<extended descriptive message>") stopc("No DESCRIPTION file found.")

The advantage is that you can wrap the extended description and also use cli formatting in it. The plain message in the stop call is then just to state the main reason of the error.

rlang conditions are better here, see ?rlang::cnd_body . Here I'm using the pattern you're suggesting, for consistency.

Thanks, looks promising.

pat-s added 29 commits October 2, 2019 22:29

save changes

efa078d

check for valid DESCRIPTION

814d31d

merge master

27dca9e

merge master

06323ab

mention how deps are installed

7048575

add vignette .org vs .com

37598fa

Merge branch 'master' into ropensci-review

34c3a94

do not use deps-in-desc hook

0d4647b

only run steps and macros interactively

e58b719

ensure that tic.R exists when calling dsl_init()

f7e8a40

fix checking for DESCRIPTION

4a6b195

file_exists -> file.exists

e8f9d71

fix stage.R

cd333e6

path_file() -> file.path()

f0efa80

forgot usethis::

1d66ea7

fix doc

7f1896b

try running tic functions within repo dir

cd722cb

run tests in repo dir

f2f9980

dont run bookdown example

84fa09b

try to solve warninsg about unimported funs

044dd38

upd desc

7d02908

dontrun do_* examples

12bf148

more dontrun

ab4598a

ignore temp pkg dir in tests

5f659ea

more dontrun

c917315

dontrun all examples

69d1021

more dontrun

5dab654

usethis: imports instead of suggests

c209c87

clean

834b30f

pat-s requested a review from krlmlr January 8, 2020 11:58

krlmlr added 7 commits January 12, 2020 20:09

Undo code changes

4740c90

Document

82c223f

Undo

18cb1ee

Undo tests

94b179f

Add new output

009dd81

Best effort

bb3fa1a

Info

2045cbe

krlmlr added 4 commits January 12, 2020 20:55

Typos

ba86b1a

Show link to help page when creating a clean stage config

da5988c

Check DESCRIPTION file for steps that need it

2648c5c

Finale

8269778

Tweak

1b95ccd

pat-s commented Jan 13, 2020

View reviewed changes

krlmlr and others added 9 commits January 15, 2020 16:54

Explain

8c1fd73

Pretty error message

0f7e58d

Link to vignettes for setup

1bc4699

ropenscilabs -> ropensci

1adacd0

add review to DESC

3272e18

use tidy desc

9cf0e6f

update links in README

a45bf8d

add codemetar file

4e045f0

acknowledge reviewers

0c066f7

pat-s changed the title ~~Address ropensci review~~ Address ropensci review Jan 15, 2020

pat-s merged commit a699f22 into master Jan 15, 2020

pat-s deleted the ropensci-review branch January 15, 2020 21:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address ropensci review #213

Address ropensci review #213

pat-s commented Jan 7, 2020 •

edited

Loading

krlmlr commented Jan 12, 2020

krlmlr commented Jan 12, 2020

krlmlr commented Jan 12, 2020

krlmlr commented Jan 12, 2020

pat-s commented Jan 13, 2020 •

edited

Loading

pat-s Jan 13, 2020

pat-s Jan 13, 2020

pat-s Jan 13, 2020

pat-s Jan 13, 2020

krlmlr Jan 15, 2020

pat-s Jan 15, 2020

Address ropensci review #213

Address ropensci review #213

Conversation

pat-s commented Jan 7, 2020 • edited Loading

krlmlr commented Jan 12, 2020

krlmlr commented Jan 12, 2020

krlmlr commented Jan 12, 2020

krlmlr commented Jan 12, 2020

pat-s commented Jan 13, 2020 • edited Loading

pat-s Jan 13, 2020

Choose a reason for hiding this comment

pat-s Jan 13, 2020

Choose a reason for hiding this comment

pat-s Jan 13, 2020

Choose a reason for hiding this comment

pat-s Jan 13, 2020

Choose a reason for hiding this comment

krlmlr Jan 15, 2020

Choose a reason for hiding this comment

pat-s Jan 15, 2020

Choose a reason for hiding this comment

pat-s commented Jan 7, 2020 •

edited

Loading

pat-s commented Jan 13, 2020 •

edited

Loading