Remove packages in non-empty namespace (#1992)

[DenisGorbachev]

Summary

This PR solves #1992.

Test plan

mkdir -p /tmp/issue-1992
cd /tmp/issue-1992
~/workspace/yarn/bin/yarn add @types/lodash
~/workspace/yarn/bin/yarn add @types/chai
~/workspace/yarn/bin/yarn remove @types/chai
ls node_modules/@types/

I've decided not to add a new test, so as not to complicate the test suite (keep testing time low).

[torifat]

I've decided not to add a new test, so as not to complicate the test suite (keep testing time low).

You don't have to worry about it.

[DenisGorbachev]

So should I add the test or is the fix sufficient by itself?

On Fri, 25 Nov 2016, 22:52 Rifat Nabi, ***@***.***> wrote: I've decided not to add a new test, so as not to complicate the test suite (keep testing time low). You don't have to worry about it. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2036 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAyoiuEd34VNZjPVrqj8ETRaNz8dBBa_ks5rBzyCgaJpZM4K8omG> .

-- @DenGorbachev <https://twitter.com/DenGorbachev>

[wyze]

A test would be helpful.

[bestander]

@DenisGorbachev, yes, definitely better add tests.
We don't want to regress and loose this fix in case of a refactoring

[DenisGorbachev]

Added tests - could you please take a look?

[torifat]

I was wondering why 311 files have changed 😕

[DenisGorbachev]

That's because I merged master into the topic branch (sorry for not doing that first before submitting the PR)

On Sun, 27 Nov 2016, 15:54 Rifat Nabi, ***@***.***> wrote: I was wondering why 311 files have changed 😕 — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#2036 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAyoilFnSxiqdfosi6AcGHOkkQ-V_gUxks5rCX2NgaJpZM4K8omG> .

-- @DenGorbachev <https://twitter.com/DenGorbachev>

[torifat]

@DenisGorbachev no worries. Just do a rebase 😄

[DenisGorbachev]

@torifat

1. I rebased my commits.
2. Actually, 311 files were changed because I added two packages in __tests__/fixtures/remove/scoped-package/node_modules/@types. It was necessary to add those packages to write the test.

[bestander]

@DenisGorbachev all those files are lodash, can you find a smaller package that will serve the purpose?
Maybe a dummy package with one file?

[DenisGorbachev]

Is it better now?

[wyze]

Yes, much better to review now.

[wyze]

I think we could avoid the subfilepath variable here and just do possibleExtraneous.add(path.join(filepath, subfile));

[DenisGorbachev]

Done; May I ask the reason?

[wyze]

I just found the variable declaration unnecessary there when only used in one place. Especially as a let. Probably wouldn't have stood out as much as a const inside the loop.

[wyze]

Looks good, thanks!

[DenisGorbachev]

Thanks everyone for reviewing and merging!