-
Notifications
You must be signed in to change notification settings - Fork 73
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add a script for generating many roxygen-examples-complete tests, apply it #1111
Conversation
.dev/gen_roxygen_tests.R
Outdated
|
||
for (test in unique_tests) { | ||
out_file <- file.path("tests", "testthat", paste0("test-roxygen-examples-complete-", test, ".R")) | ||
extras <- if (test == "15") ', scope = "spaces"' else "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid magic string here? What's special about "15"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL
#1110 is merged, so I am going to drop this from the title. Feel free to rebase. |
@IndrajeetPatil since you started to review, I leave this to you. |
@MichaelChirico due to #1119, we need to release a new version of {styler} soon (mid next week), just if you want this PR to be included. I don't think we'll have another release soon after that. |
Thanks for the ping, feedback handled. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1111 +/- ##
=======================================
Coverage 92.31% 92.31%
=======================================
Files 46 46
Lines 2655 2655
=======================================
Hits 2451 2451
Misses 204 204 ☔ View full report in Codecov by Sentry. |
Thanks. @IndrajeetPatil I leave the review to you, only thing I would add is that I am not a big fan of adding a top level directory (albeit hidden) to the repo. Or do we expect future scripts to live in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @MichaelChirico! I like the idea of automating the generation of these test files.
But I agree with Lorenz that it does feel a bit awkward to store the relevant scripts at the top-level of the package directory. I use and see the value of /.dev
, but I would use it only for tasks that relate to the entire package (e.g. benchmarking, revdep, etc.). I would instead store the relevant scripts in tests/dev
folder.
Does that sound reasonable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
The precommit workflow is going to fail until #1158 is merged, and so need to wait for that PR to be merged. |
Since we moved the script to generate the tests, some code comments still point to old directory… |
Closes #1105. Note that this includes #1110 so the diff is too large for now. I couldn't figure out a way to specify that commit as the diffbase while still creating this PR on r-lib/styler (since that commit is on my fork). Sorry for the visual noise.