- Notifications
You must be signed in to change notification settings - Fork85
.rscignore update for .gitignore functionality#1185
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Implement full gitignore pattern syntax (wildcards, negation, relative paths,double-asterisk) while maintaining backward compatibility and existinghierarchical behavior. Includes comprehensive test suite.
Introduce breaking changes to .rscignore files, enabling hierarchical pattern matching similar to .gitignore. Patterns in parent directories now affect subdirectories, allowing for more intuitive file ignoring. Added migration guidance and temporary compatibility option for legacy behavior. Update documentation and tests to reflect these changes.
Move existing tests for .rscignore functionality, including gitignore-style patterns and hierarchical behavior, to a new dedicated test file.Revert test-bundleFiles.R to original state.
R/bundleFiles.R Outdated
| if (legacy_mode) { | ||
| # Issue deprecation warning for legacy behavior | ||
| lifecycle::deprecate_warn( | ||
| when="1.6.0", |
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.
unclear when this should be.
| "Directory-scoped .rscignore patterns are deprecated.", | ||
| "Hierarchical .gitignore-style behavior is now the default.", | ||
| "Update your .rscignore files to use '!' negation patterns if needed.", | ||
| "Set `options(rsconnect.rscignore.legacy = FALSE)` to remove this warning." |
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.
Happy for feedback on what this message would say
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.
Since people will only see this if they've explicitly opted into the legacy behavior, I think we don't need to tell them what option to use to silence it. Could we instead include a link to the documentation that describes the change and how to update your .rscignore files?
Update to be more specific about what patterns are and are not supported in the new implementation.Refresh package documentation.
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.
happy to integrate this (include these) into the test-bundleFiles.R to be consistent with normal approach.
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.
Yes, that would be great, thanks!
karawoo left a comment
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.
Hi@BrianLang, thank you so much for taking this on! This feels really on the right track and I appreciate the effort and the thorough test coverage.
I know this PR is a draft still, but I hope you don't mind me chiming in early with some comments. Feel free to disregard for now if you're still working on things, and you can tag me back in whenever you're ready for another look.
| ignoreFiles=TRUE | ||
| ignoreFiles=TRUE, | ||
| bundle_root=rootDir | ||
| ) { | ||
| # generate a list of files at this level | ||
| if (is.null(contents)) { | ||
| contents<- list.files(dir,all.files=TRUE,no..=TRUE) | ||
| } | ||
| if (ignoreFiles) { | ||
| contents<- ignoreBundleFiles(dir,contents) | ||
| contents<- ignoreBundleFiles(dir,contents,bundle_root) |
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.
Do we need the newbundle_root argument to this function or could we pass the existingrootDir argument toignoreBundleFiles instead?
| # Handle NULL case | ||
| if (is.null(legacy_mode)) { | ||
| legacy_mode<-FALSE | ||
| } | ||
| if (legacy_mode) { |
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.
| # Handle NULL case | |
| if (is.null(legacy_mode)) { | |
| legacy_mode<-FALSE | |
| } | |
| if (legacy_mode) { | |
| if (isTRUE(legacy_mode)) { |
this way would be just a little simpler
| "Directory-scoped .rscignore patterns are deprecated.", | ||
| "Hierarchical .gitignore-style behavior is now the default.", | ||
| "Update your .rscignore files to use '!' negation patterns if needed.", | ||
| "Set `options(rsconnect.rscignore.legacy = FALSE)` to remove this warning." |
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.
Since people will only see this if they've explicitly opted into the legacy behavior, I think we don't need to tell them what option to use to silence it. Could we instead include a link to the documentation that describes the change and how to update your .rscignore files?
| if (is.null(bundle_root)) { | ||
| bundle_root<-dir# Fallback for backward compatibility | ||
| } |
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.
Is there any reason not to have thebundle_root argument default todir instead of setting it here?
| tryCatch({ | ||
| patterns<- collectHierarchicalPatterns(dir,bundle_root) | ||
| if (length(patterns)>0) { | ||
| contents<- applyIgnorePatterns(contents,patterns,dir) | ||
| } | ||
| # Always exclude .rscignore files themselves | ||
| contents<- setdiff(contents,".rscignore") | ||
| return(contents) | ||
| },error=function(e) { | ||
| # Fallback to directory-scoped behavior on error | ||
| warning("Error in hierarchical pattern processing:",e$message,call.=FALSE) | ||
| warning("Falling back to directory-scoped patterns",call.=FALSE) | ||
| return(applyDirectoryScopedPatterns(contents,dir)) | ||
| }) | ||
| } |
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.
I think thistryCatch() has the potential to obfuscate the behavior a little too much. I think we should go ahead and fail fast if an error occurs, and the error message can point people to thersconnect.rscignore.legacy option.
| # Later patterns override earlier patterns | ||
| for (patterninpatterns) { | ||
| for (fileinfile_list) { | ||
| # Hierarchical pattern matching logic (formerly in matchPatternHierarchical) |
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.
| # Hierarchical pattern matching logic (formerly in matchPatternHierarchical) |
seems like this references a previous implementation that's not relevant anymore
| if (pattern$relative&& startsWith(pattern$raw,"/")) { | ||
| # Get the directory containing this pattern's .rscignore file | ||
| pattern_dir<-pattern$source_dir | ||
| # Get the directory containing the current file | ||
| if (grepl("/",file)) { | ||
| file_dir<- file.path(current_dir, dirname(file)) | ||
| }else { | ||
| file_dir<-current_dir | ||
| } | ||
| # Normalize paths for comparison | ||
| pattern_dir<- normalizePath(pattern_dir,mustWork=FALSE) | ||
| file_dir<- normalizePath(file_dir,mustWork=FALSE) | ||
| # If directories match, compare just the filename | ||
| if (pattern_dir==file_dir) { | ||
| file_basename<- basename(file) | ||
| matches<- matchPattern(file_basename,pattern,current_dir) | ||
| } | ||
| }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.
Is there any reason not to make this logic part ofmatchPattern?
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.
Yes, that would be great, thanks!
| expect_false("temp.txt"%in%result)# Root: ignored | ||
| expect_true("src/temp.txt"%in%result)# Child: un-ignored (overrides parent) | ||
| expect_false("src/utils/temp.txt"%in%result)# Grandchild: re-ignored (overrides parent negation) | ||
| expect_true("src/main.R"%in%result)# Unaffected |
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.
awesome 🎉
| #' * You can exclude additional files by listing them in a `.rscignore` | ||
| #' file. This file follows .gitignore-style syntax with one pattern per line. | ||
| #' Patterns support wildcards (* and ?), directory patterns (dir/), and | ||
| #' negation (!pattern). Character classes ([abc], [0-9]) and brace expansion | ||
| #' ({a,b,c}, {1..3}) are not currently supported. Patterns in parent | ||
| #' directories affect subdirectories hierarchically. |
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.
| #' * You can exclude additional files by listing them in a `.rscignore` | |
| #' file. This file follows .gitignore-style syntax with one pattern per line. | |
| #' Patterns support wildcards (* and ?), directory patterns (dir/), and | |
| #' negation (!pattern). Character classes ([abc],[0-9]) and brace expansion | |
| #'({a,b,c},{1..3}) are not currently supported. Patterns in parent | |
| #' directories affect subdirectories hierarchically. | |
| #' * You can exclude additional files by listing them in a `.rscignore` | |
| #' file. This file follows .gitignore-style syntax with one pattern per line. | |
| #' Patterns support wildcards (* and ?), directory patterns (dir/), and | |
| #' negation (!pattern). Character classes (`[abc]`, `[0-9]`) and brace | |
| #'expansion (`{a,b,c}`, `{1..3}`) are not currently supported. Patterns in | |
| #'parentdirectories affect subdirectories hierarchically. |
The brackets here were causing r cmd check complaints, which should be fixed by code formatting them.
❯ checking Rd cross-references ... WARNING Missing link(s) in Rd file 'listDeploymentFiles.Rd': ‘abc’ ‘0-9’ See section 'Cross-references' in the 'Writing R Extensions' manual. Found the following Rd file(s) with Rd \link{} targets missing package anchors: listDeploymentFiles.Rd: abc, 0-9 Please provide package anchors for all Rd \link{} targets not in the package itself and the base packages.❯ checking Rd files ... NOTE checkRd: (-1) listDeploymentFiles.Rd:47: Lost braces; missing escapes or markup? 47 | ({a,b,c}, {1..3}) are not currently supported. Patterns in parent | ^ checkRd: (-1) listDeploymentFiles.Rd:47: Lost braces; missing escapes or markup? 47 | ({a,b,c}, {1..3}) are not currently supported. Patterns in parent | ^BrianLang commentedJul 31, 2025
Thanks so much for the thorough review, i appreciate the time you took, I will address them in the coming days. |
Hierarchical .rscignore Implementation
Summary
This PR implementshierarchical .gitignore-style behavior for
.rscignorefiles as discussed in#1178, making them work like.gitignorefiles where parent directory patterns affect subdirectories and allowing general wildcard matching as in.gitignorefiles.Key Changes
Core Behavior Change
.rscignorepatterns only affected files in the same directory and use literal string matching to select ignored files..rscignorepatterns inherit to subdirectories and support most .gitignore supported patterns (see below)SUPPORTED Features
Basic Patterns
Hierarchical Behavior
Advanced Patterns
Examples That Work
NOT SUPPORTED Features
Advanced Pattern Syntax
Implementation Details
New Functions Added
collectHierarchicalPatterns()- Collects patterns from current directory up to project rootapplyRscignorePatterns()- Enhanced to use hierarchical patterns with fallbackapplyRscignorePatternsLegacy()- Preserves old behavior during transitionModified Functions
bundleFiles()- Now passes bundle root contextignoreBundleFiles()- Uses hierarchical pattern collectionPattern Precedence Rules
.rscignorepatterns take precedence!patternin child can un-ignore parent'spatternLifecycle Management
Breaking Change Strategy
Following RStudio/Posit patterns for managed breaking changes:
Test Coverage
Files Modified
Core Implementation
R/bundleFiles.R- Main hierarchical logic and lifecycle managementman/*.Rd- Documentation for new exported functionsTest Suite
tests/testthat/test-hierarchical-rscignore.R- Comprehensive hierarchical behavior teststests/testthat/_snaps/bundleFiles.md- Updated snapshots for new behaviorDocumentation
NEWS.md- Suggestion for breaking change announcement and migration guide