-
Notifications
You must be signed in to change notification settings - Fork 2
FEA-2048: Ignore nested subpackages #60
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
lib/src/utils.dart
Outdated
return Directory(rootDirectory) | ||
.list(recursive: true, followLinks: false) | ||
.where((file) => p.basename(file.path) == 'pubspec.yaml') | ||
.map((file) => file.path) | ||
.toList(); |
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 tested the performance of this on a few different large dart packages
For a package that had 65,000 files, this took 300ms. An acceptable performance hit for a repo of that size
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
repo: ["Workiva/over_react", "rrousselGit/provider", "dart-lang/args"] | ||
repo: [ | ||
"Workiva/over_react", | ||
"Workiva/w_module", |
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.
w_module has a nested pubspec.yaml
file in example
Add it as a consumer to verify this new functionality of not breaking on nested packages
textDocumentEncoding: TextEncoding.UTF8, | ||
toolInfo: ToolInfo( | ||
name: 'scip-dart', | ||
version: '0.0.1', |
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.
Unrelated but should this match the version of this library? Do we need to move this to a constant somewhere and configure the release event to bump it as well?
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.
Yea, ideally we do
I didn't love the idea of relying on a release event (if for whatever reason scip-dart changed owners outside of workiva one day) and wanted the solution to be baked into the project
I have an issue tracking this here and a spike pr for one of the solutions here: #45. Just haven't pushed on wrapping that up yet
QA +1 CI passes and verified that indexing works on some packages locally |
🚀 @Workiva/release-management-p 🚢 |
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.
+1 from RM
FEA-2048
Closes #59
Running
scip-dart
on a dart package with a nested pubspec.yaml file would result in an error (see #59), this pr ignores any subdirectories that declare their own pubspec.yaml fileIf nested directories want to be indexed as well, scip-dart can just be re-run on that path
Also, as a QoL update, if the root path provided to
scip_dart ./path
did not contain a pubspec.yaml file, the cli logs an error and fails with a non-zero exit code.some CI changes are in this pr as well: