-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Remove packages in non-empty namespace (#1992) #2036
Remove packages in non-empty namespace (#1992) #2036
Conversation
You don't have to worry about it. |
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>
|
A test would be helpful. |
@DenisGorbachev, yes, definitely better add tests. |
Added tests - could you please take a look? |
I was wondering why |
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>
|
@DenisGorbachev no worries. Just do a rebase 😄 |
|
@DenisGorbachev all those files are lodash, can you find a smaller package that will serve the purpose? |
Is it better now? |
Yes, much better to review now. |
let subfilepath; | ||
for (const subfile of subfiles) { | ||
subfilepath = path.join(filepath, subfile); | ||
possibleExtraneous.add(subfilepath); |
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 we could avoid the subfilepath
variable here and just do possibleExtraneous.add(path.join(filepath, subfile));
Done; May I ask the reason? |
I just found the variable declaration unnecessary there when only used in one place. Especially as a |
Looks good, thanks! |
Thanks everyone for reviewing and merging! |
Summary
This PR solves #1992.
Test plan
I've decided not to add a new test, so as not to complicate the test suite (keep testing time low).