Fix --ignore flag when globs are expanded to an array

[rtsao]

Suppose we have:

• package-a
• package-b
• package-c

---

The following works as expected:

lerna bootstrap --ignore=package-@(a|b)

The message is:

Ignoring packages that match 'package-@(a|b)'

And the following packages are bootstrapped:

• package-c

---

The following does not work as expected:

lerna bootstrap --ignore=package-{a,b}

The message is:

Ignoring packages that match 'package-a,package-b'

And the following packages are bootstrapped:

• package-a (incorrect)
• package-b (incorrect)
• package-c

The latter glob is expanded to an array even before reaching filterPackage in PackageUtilities. (Notice the difference in messages). However, the logic regarding negation (i.e. maybeNegate) was only within glob.some, which would not work in the case that glob.length > 1.

This PR changes the boolean reducer depending on the desired negation (i.e. Array.prototype.some for the standard case, and Array.prototype.every for the negated case.

[evocateur]

Thanks @rtsao for this beautiful PR! I did not know brace expansions in CLI flags would get interpreted like this.

I'd like to get @gigabo's eyes on this before we merge it, as he was the last to touch this part of the code directly.

[rtsao]

At a high-level, it seems somewhat weird to me that package-{a,b} gets expanded to an array but package-@(a|b) does not. I'm not sure why this is the case.

That said, the existing logic was clearly designed to handle arrays (it just so happens it didn't quite handle arrays correctly in the negation case and there were no tests for the array case) so I figured fixing this bug in this way seemed appropriate.

[gigabo]

Thanks for the patch @rtsao!

it seems somewhat weird to me that package-{a,b} gets expanded

That is weird. Is the shell expanding it?

Are you in a directory where ./package-a/ and ./package-b/ are direct children?

didn't quite handle arrays correctly in the negation case

Nice catch! Thanks for adding a test for it, too. 🎉

[gigabo]

Hmmm... this is getting a little roundabout. Maybe simpler at this point to just have a top-level if/else?

Like:

if (negate) {
  return glob.every((glob) => !minimatch(pkg.name, glob));
} else {
  return glob.some((glob) => minimatch(pkg.name, glob));
}

[rtsao]

I thought about doing that but didn't want to rock the boat in terms of the existing code. Agreed, that's much simpler, so I'll update the PR with that refactor. 👍

[gigabo]

Could maybe simplify to make it easier to read, but good fix! 👍

[lock]

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.