-
Notifications
You must be signed in to change notification settings - Fork 10
remove with_dir() wrapper from a11y fxns' examples #413
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
Conversation
New version checklist
|
Checklist
|
Code Metrics Report
Code coverage of files in pull request scope (0.0%)
Reported by octocov |
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.
Pull request overview
This PR removes the withr::with_dir() wrapper from accessibility-related functions (add_tagging, add_alttext, add_accessibility) in both examples and tests. The functions now accept the full directory path directly via the dir parameter instead of changing the working directory.
Changes:
- Removed
withr::with_dir()wrappers from function examples and tests - Updated
dirparameter to usefile.path(getwd(), "report")instead of relying on directory context switching - Removed unnecessary
pathvariable assignments in examples
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/testthat/test-add_tagging.R | Removed withr::with_dir() wrapper and updated dir parameter in commented test code |
| tests/testthat/test-add_alttext.R | Removed withr::with_dir() wrapper and updated dir and related directory parameters in commented test code |
| tests/testthat/test-add_accessibility.R | Removed withr::with_dir() wrapper and updated dir and related directory parameters in commented test code |
| man/add_tagging.Rd | Updated documentation example to remove withr::with_dir() wrapper and unnecessary path variable |
| man/add_alttext.Rd | Updated documentation example to remove withr::with_dir() wrapper and unnecessary path variable |
| man/add_accessibility.Rd | Updated documentation example to remove withr::with_dir() wrapper and unnecessary path variable |
| R/add_tagging.r | Updated function documentation example to remove withr::with_dir() wrapper and unnecessary path variable |
| R/add_alttext.r | Updated function documentation example to remove withr::with_dir() wrapper and unnecessary path variable |
| R/add_accessibility.R | Updated function documentation example to remove withr::with_dir() wrapper and unnecessary path variable |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Could you remind me why we are removing this from the function? By using with_dir, the user doesn't have to set their WD to the report file to render. The function is using the dir in the args. Maybe I'm too deep into this and am misunderstanding/forgetting? |
When you last updated the a11y fxns, you edited them so that the wrapper, nor setting the "report" folder as the wd, is necessary. I checked with all three a11y fxns. You can run the examples to double-check! |
|
@sbreitbart-NOAA Ohh yes thank you! I wasn't realizing that these were changes to the examples, not the code! Makes more sense. This looks good to me then! |
|
@Schiano-NOAA I realized the PR title and description didn't clearly say the changes were to the examples, not the code itself. My bad! Updated those things. Will merge. |
What is the feature?
How have you implemented the solution?
Does the PR impact any other area of the project, maybe another repo?