-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[clean] Support --include-filtered-dependencies flag #543
Conversation
let curPkg = 1; | ||
stub(FileSystemUtilities, "rimraf", (actualDir, callback) => { | ||
const expectedDir = path.join( | ||
testDir, "packages/package-" + curPkg, "node_modules" |
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 if you used path.join here it'd fix the appveyor failures
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 suspect it would. Good call.
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.
Added path.join
down around line 128. I also realized I oopsidentally checked in a describe.only
, so I removed that too 😁
ffa4c69
to
8970c8f
Compare
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.
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.
LGTM
import PromptUtilities from "../PromptUtilities"; | ||
import progressBar from "../progressBar"; | ||
|
||
export default class CleanCommand extends Command { | ||
initialize(callback) { | ||
this.packagesToClean = this.filteredPackages; | ||
if (this.flags.includeFilteredDependencies) { | ||
this.packagesToClean = PackageUtilities.addDependencies(this.packagesToClean, this.packageGraph); |
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 was about to ask why not just mutate this.filteredPackages
in place, but then I realized this is a pattern used elsewhere and it makes it more readable to boot. 👓
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.
Yep, I mostly just blindly copied the pattern. I think if this were to be extended other commands in the future, it could make sense to pull the check up into Command.js, and modify the filteredPackages
there, maybe? I haven't looked closely at that though.
Yeah, no worries for now, mostly just recording for myself that this was in evidence elsewhere.
We'll likely have to do a fuller analysis of all the flags and such, not sure _all_ the commands need such filtering...
… On Jan 29, 2017, at 09:14, Robert Gay ***@***.***> wrote:
@roblg commented on this pull request.
In src/commands/CleanCommand.js:
> import PromptUtilities from "../PromptUtilities";
import progressBar from "../progressBar";
export default class CleanCommand extends Command {
initialize(callback) {
+ this.packagesToClean = this.filteredPackages;
+ if (this.flags.includeFilteredDependencies) {
+ this.packagesToClean = PackageUtilities.addDependencies(this.packagesToClean, this.packageGraph);
Yep, I mostly just blindly copied the pattern. I think if this were to be extended other commands in the future, it could make sense to pull the check up into Command.js, and modify the filteredPackages there, maybe? I haven't looked closely at that though.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.
|
This thread has been automatically locked because there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Sorry about the horkage in the CleanCommand test. I moved the existing tests into their own
describe
, which caused whitespace changes, which... you can see the results.