Skip to content

Commit

Permalink
fix(filter-options): Require arguments to --scope and --ignore
Browse files Browse the repository at this point in the history
  • Loading branch information
@evocateur
evocateur committed Feb 7, 2019
1 parent c2c529d commit 4b81dad
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 0 deletions.
17 changes: 17 additions & 0 deletions core/filter-options/__tests__/get-filtered-packages.test.js
View file
Expand Up @@ -34,6 +34,23 @@ beforeAll(async () => {
cwd = await initFixture("filtering");
});

test.each`
flag
${"--scope"}
${"--ignore"}
`("$flag requires an argument", async ({ flag }) => {
// swallow stderr during yargs execution
jest.spyOn(console, "error").mockImplementation(() => {});

try {
parseOptions(flag);
} catch (err) {
expect(err.message).toMatch("Not enough arguments");
}

expect.hasAssertions();
});

test.each`
argv | matched
${["--scope", "package-3"]} | ${[3]}
Expand Down
2 changes: 2 additions & 0 deletions core/filter-options/index.js
View file
Expand Up @@ -12,10 +12,12 @@ function filterOptions(yargs) {
scope: {
describe: "Include only packages with names matching the given glob.",
type: "string",
requiresArg: true,
},
ignore: {
describe: "Exclude packages with names matching the given glob.",
type: "string",
requiresArg: true,
},
"no-private": {
describe: 'Exclude packages with { "private": true } in their package.json.',
Expand Down

5 comments on commit 4b81dad

@quantizor
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change...

@evocateur
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How exactly were these flags functional without arguments?

@quantizor
Copy link

@quantizor quantizor commented on 4b81dad Feb 13, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It allowed you to set up the command as an npm script and then optionally filter only if the arg was actually passed.

e.g.

{
  "scripts": {
    "test": "lerna run test --since --scope",
  }
}

So if you run npm test it tests all the ones that have changed, but if you run npm test foo it will only test the "foo" package.

@evocateur
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not the way you pass ad-hoc flags to an npm script, you have to use -- to stop npm from parsing them. Your example only works for unrecognized positional arguments, and it's not a very robust pattern in any case.

npm test -- --scope <glob>

In general, if there isn't a test for it, it isn't a feature. As there was never a test for the previous behavior among over 700 tests currently extant, I don't consider it part of the public API, thus it is not a semver breaking change.

@quantizor
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair, I use yarn and I think they do the -- thing on your behalf for scripts so didn't think to include it in my example.

In general, if there isn't a test for it, it isn't a feature.

Understood, thanks.

Please sign in to comment.