Getting started vignette #60

assignUser · 2021-09-13T17:22:19Z

Closes #42

Let me know what you think :)

had to uninstall precommit to be able to commit the readme changes because the hook uses bash (but it looks like there are already multiple issues related to it so i wont open one 👍 ).

lorenzwalthert · 2021-09-13T18:03:55Z

Cool. Thanks, I’ll look at it later this week tie earliest unfortunately.

You can git commit —no-verify or SKIP=hook-id git commit to skip hooks as per the documentation. We‘ll switch to R based hook soon so tue path issues won’t persist forever.

lorenzwalthert · 2021-09-16T19:40:39Z

vignettes/touchstone.Rmd

+touchstone::refs_install() # installs branches to benchmark
+touchstone::pin_assets("data/all.Rdata", "inst/scripts") # pin files and directories
+
+load(path_pinned_asset("all.Rdata")) # perform other setup you need for the benchmarks


Ideally, this would actually be load(path_pinned_asset("all.Rdata")), don't you think? Do we place all assets in the temp directory root?

I am not quite sure I understand, that's what it says?

We have separate dirs in the temp dir for head and base ref now that we allow pinning from both to avoid name collisions.

We have separate dirs in the temp dir for head and base ref now that we allow pinning from both to avoid name collisions.

Yes, but if I pin data/all.Rdata, it would be most natural to access it with path_pinned_asset("data/all.Rdata") instead of path_pinned_asset("all.Rdata"). Do you agree or not?

@assignUser?

I'll plan some time for this and the other issue this coming week :)

Great. Thanks to your help, we have #64 and are ready for a CRAN submission after this PR and #63.

We only want this to work for subdirectories of the current working directory/git repo right? So a path like ./../../../some/dir should be invalid assuming . is the root of the git repo, right?

I agree it should be invalid. And all paths should be relative to the repo root. For other patients logic, do we also take the repo root as a starting point or are some things relative to touchstone/? This should be consistent across the API.

@lorenzwalthert This should now work as we expect, I added some handling of the repo root as we only did this implicitly up to now, please check if the way I implemented this is ok for you. I still need to update the documentation at some points etc. put functionally it should be ok ) also added/modified the tests.

lorenzwalthert · 2021-09-16T19:41:02Z

vignettes/touchstone.Rmd

+touchstone::pin_assets("data/all.Rdata", "inst/scripts") # pin files and directories
+
+load(path_pinned_asset("all.Rdata")) # perform other setup you need for the benchmarks
+source(path_pinned_asset("scripts/prepare.R"))


potentially confusing for the user because this script is not pinned...

I choose these examples to illustrate that you can pin both files and directories and how only the last dir is copied when you use nested dirs.

only the last dir is copied

Ok, this is probably related to what I wrote above. I think the whole directory tree should be copied, not just the last directory.

Can we change that?

lorenzwalthert · 2021-10-17T20:18:34Z

@assignUser thanks, I can have a look at resolving the merge conflicts now...

for more information, see https://pre-commit.ci

lorenzwalthert · 2021-10-17T22:19:17Z

I think there are a few related things that we should improve:

touchstone.assets_dir* should be set in .onLoad() to make things more flexible (also testing)
{withr} statements should not be nested if possible for easier error identification.
I believe the assets are not correctly copied into the right directory, it's a bit tricky.
more tests for copying
running locally and on CI/CD creates new snaps for macOS and Windows, I don't know why...

I will look into this next week hopefully.

lorenzwalthert · 2021-10-17T22:19:51Z

Thanks for the work you did so far @assignUser 👍

assignUser · 2021-10-18T10:07:36Z

* running locally and on CI/CD creates new snaps for macOS and Windows, I don't know why...

~~This was an issue with the new git_rootoption not being set in the test, I fixed that and also un-nested the withr calls.~~ Actually this seems to be caused by the temp paths being different on the mac/windows runners for some reason:

 - "C:/Users/runneradmin/AppData/Local/Temp/RtmpYb4HIg/working_dir/RtmpW0XVk4/with-git"
+ "C:/Users/RUNNER~1/AppData/Local/Temp/RtmpYb4HIg/working_dir/RtmpW0XVk4/with-git"

- "/private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/RtmpKkiy5N/working_dir/RtmpacJFHY/with-git"
+ "/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/RtmpKkiy5N/working_dir/RtmpacJFHY/with-git"

I am looking into it

lorenzwalthert · 2021-10-18T10:11:22Z

ok, thanks. I'll look into the other things then...

lorenzwalthert · 2021-10-18T10:25:50Z

I had the tempfile problem before somewhere else. I just deactivated the test for Windows: 11cfc5e. We could also opt against using a snap and do a regex match of the path instead where we try to match the part after /private/var (for macOS).

assignUser · 2021-10-18T10:33:46Z

Yeah the reason seems to be (on windows, idk why it does this on mac? Ihave no mac myslef to test that behaviour nor any experience with macos)
r-lib/actions#217
actions/runner-images#712

assignUser · 2021-10-18T11:05:06Z

re:mac it appears that /var/folder is a sym link to /private/var/folders so the issue was most likely caused by my use of fs::path_real which replaces sym links. I now modified the tests to also use path_real to remove the windows shortname/mac sym link. And that seems to have worked :)

assignUser · 2021-10-18T11:21:13Z

touchstone.assets_dir* should be set in .onLoad() to make things more flexible (also testing)
{withr} statements should not be nested if possible for easier error identification.
I believe the assets are not correctly copied into the right directory, it's a bit tricky.
more tests for copying <- can't confirm, locally it works as intended and tests seem to also be fine?
running locally and on CI/CD creates new snaps for macOS and Windows, I don't know why...

lorenzwalthert · 2021-10-18T11:40:19Z

Great @assignUser 👍, thanks a lot. I'll discuss my concerns in s separate issue I think, since this is on the brink of going off topic 😄

assignUser added 3 commits September 13, 2021 16:02

add dir to rbuildignore

cabc386

add how to vignette

5a70439

update snapshot

6ba2261

expand vignette, update screenshot

240b65c

assignUser mentioned this pull request Sep 14, 2021

cran release #31

Open

12 tasks

lorenzwalthert added 2 commits September 16, 2021 21:43

minor edits

30c394d

vignette similar to styler

c268c7d

lorenzwalthert reviewed Sep 17, 2021

View reviewed changes

add git root handeling

d2aa981

lorenzwalthert and others added 3 commits October 17, 2021 22:28

Merge branch 'main' into new-vignette

13d49d8

[pre-commit.ci] auto fixes from pre-commit.com hooks

a589c80

for more information, see https://pre-commit.ci

drop random word

ed7b738

clean up and fix tests

1bd2bc2

assignUser added 2 commits October 18, 2021 12:20

change to fs

00c9f31

update doc

be84524

assignUser added 2 commits October 18, 2021 12:45

remove fs::path_real

2c1c60a

use fs::path_real in tests

0cdd7b9

set asset dirs onLoad

e91150e

lorenzwalthert merged commit cc98ff7 into lorenzwalthert:main Oct 18, 2021

assignUser deleted the new-vignette branch October 18, 2021 17:31

lorenzwalthert mentioned this pull request Oct 19, 2021

Asset pinning not working as expected #68

Closed

assignUser mentioned this pull request Nov 3, 2021

Document common pitfalls #29

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Getting started vignette #60

Getting started vignette #60

assignUser commented Sep 13, 2021

lorenzwalthert commented Sep 13, 2021

lorenzwalthert Sep 16, 2021

assignUser Sep 17, 2021

lorenzwalthert Sep 17, 2021

lorenzwalthert Oct 10, 2021

assignUser Oct 10, 2021

lorenzwalthert Oct 10, 2021

assignUser Oct 16, 2021

lorenzwalthert Oct 16, 2021

assignUser Oct 17, 2021

lorenzwalthert Sep 16, 2021

assignUser Sep 17, 2021

lorenzwalthert Sep 17, 2021

lorenzwalthert Sep 17, 2021

lorenzwalthert commented Oct 17, 2021 •

edited

Loading

lorenzwalthert commented Oct 17, 2021

lorenzwalthert commented Oct 17, 2021

assignUser commented Oct 18, 2021

lorenzwalthert commented Oct 18, 2021

lorenzwalthert commented Oct 18, 2021 •

edited

Loading

assignUser commented Oct 18, 2021

assignUser commented Oct 18, 2021

assignUser commented Oct 18, 2021

lorenzwalthert commented Oct 18, 2021

Getting started vignette #60

Getting started vignette #60

Conversation

assignUser commented Sep 13, 2021

lorenzwalthert commented Sep 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorenzwalthert commented Oct 17, 2021 • edited Loading

lorenzwalthert commented Oct 17, 2021

lorenzwalthert commented Oct 17, 2021

assignUser commented Oct 18, 2021

lorenzwalthert commented Oct 18, 2021

lorenzwalthert commented Oct 18, 2021 • edited Loading

assignUser commented Oct 18, 2021

assignUser commented Oct 18, 2021

assignUser commented Oct 18, 2021

lorenzwalthert commented Oct 18, 2021

lorenzwalthert commented Oct 17, 2021 •

edited

Loading

lorenzwalthert commented Oct 18, 2021 •

edited

Loading