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

Snapshot hint override #1608

Merged
merged 8 commits into from
Apr 25, 2022
Merged

Snapshot hint override #1608

merged 8 commits into from
Apr 25, 2022

Conversation

hadley
Copy link
Member

@hadley hadley commented Apr 21, 2022

Fixes #1606

@hadley hadley requested a review from gaborcsardi April 21, 2022 16:37
@gaborcsardi
Copy link
Member

I think the test needs to depend on the new cli version, to fix the CI failures, which run with old cli.

@hadley
Copy link
Member Author

hadley commented Apr 24, 2022

Oops, I forgot to check that it had actually been accepted by CRAN.

@gaborcsardi
Copy link
Member

Unrelated to this PR, but seems like the variants are not working properly here: https://github.com/r-lib/testthat/runs/6147007072?check_suite_focus=true#step:7:180

Maybe when our CI release switches to R-release really that will fix this.

@@ -1,5 +1,7 @@
# testthat (development version)

* Fix bug revealed by fix to another bug in cli (#1606).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this is very useful to the end user, maybe we don't need to mention it at all, as nobody has really seen this bug?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, not sure why I wrote that. I'll just add something generic about cli.

* Run `snapshot_accept('bar.R')` to accept the change
* Run `snapshot_review('bar.R')` to interactively review the change
* Run `]8;;rstudio:run:testthat::snapshot_accept('bar.R')snapshot_accept('bar.R')]8;;` to accept the change
* Run `]8;;rstudio:run:testthat::snapshot_review('bar.R')snapshot_review('bar.R')]8;;` to interactively review the change
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cli is not using the ideal ansi sequence for links currently, because rstudio does not support that yet. But once we fix this in rstudio, cli will switch to that as well, so these snapshots will change. fyi.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, sounds good.

@hadley hadley merged commit 0f24eae into main Apr 25, 2022
@hadley hadley deleted the snapshot-hint-override branch April 25, 2022 13:23
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.

local_reproducible_context() is active when the snapshot advice is printed
2 participants