[Feature] Adds --scope and --ignore support for bootstrap, exec, run, clean and ls by lukebatchelor · Pull Request #386 · lerna/lerna

lukebatchelor · 2016-10-21T05:14:31Z

Some refactoring to make --scope and --ignore more consistent across commands.

Fixes #383

lukebatchelor · 2016-10-21T07:29:39Z

So, this was based on asini/asini#8 which had some other changes not in Lerna which I've just realised came from #365. Lots of this was verbatim from the asini merge but there are a couple of differences from what was taken from #365. More than happy to help merge these two together in which ever order, just give me a yell. 👍

lukebatchelor · 2016-10-21T07:55:44Z


  runPreparations() {
+    const scope = this.flags.scope || (this.configFlags && this.configFlags.scope);
+    const ignore = this.flags.ignore || (this.configFlags && this.configFlags.ignore);


The .configFlags is only set in Bootstrap right now.

lukebatchelor · 2016-10-21T08:00:38Z


-The `ignore` flag, when used with the `bootstrap` command, can also be set in `lerna.json` under the `bootstrapConfig` key. The command-line flag will take precendence over this option. This flag is supported in `bootstrap` and `exec` commands.
-
-**Note**: If both `scope` and `ignore` are provided to `exec` command, `scope` takes precedence.


An intended side effect of this change is that scope wont take precedence anymore. Is there any reason to take precedence when you could simply obey both and throw an error if they would result in no packages being selected? I don't think it would be a common use case to be using both at the same time, but this would make more sense to me.

(More than happy to change it though, just felt more consistent)

In life in general, explicit exclusions tend to take precedence over explicit inclusions. I think this is a reasonable change. I would perhaps print a warning/confirmation that X number of packages would be in scope but were excluded? Better to be explicit with your user. Does it still throw if no packages are selected?

Yup, still throws the error.

Yea, a warning might be nice for that. If you were using both at the same time I imagine you must already know that they will have overlap (or you would have just used 1 right?), but it would still make sense from a logging point of view

Scoping to packages that match "package-1*" Ignoring packages that match "*-a" 1 package would have been scoped but is ignored: -- "package-1-a"

True, you would presumably know what you're getting yourself into. Lerna doesn't log the names anywhere else, so a count is probably sufficient.

@lukebatchelor When we developed the feature I suggested ignore to trump scope: #168 (comment) - not sure why and when that might have changed.

@joscha It changed in this PR, hopefully. Looking through that convo, I don't see a reason you cant accept both flags (as in this PR) and throw an error if the result would return no packages.

Useful for cases like

npm install --ignore-scripts && lerna bootstrap --scope "ak*" --ignore "ak-editor*"

Ignore should definitely take precedence over scope.

Just to make sure we're saying the same thing, scope wont override ignore in this case. They work together. Scope is a whitelist, ignore is a blacklist. If scope exists, then no other packages other than the ones in scope are included. If ignore exists, no packages matched by ignore will get included.

So, if you were matched by both, you would still get ignored.

Yeah, that's right.

lukebatchelor · 2016-10-23T22:30:18Z

@hzoo, @thejameskyle, Is there anything else I should do for this to get merged? I've got the next 3 days to be able to work on this pretty freely, and was hoping to do any iteration on this one, starting work merging it with #365 (my PR borrows quite a lot of the refactoring from that), and looking how we might do something with #388, probably/possibly using #384

seansfkelley · 2016-11-01T23:15:54Z

@hzoo / @thejameskyle can we merge this? Working around/waiting for prepublishing the entire world is making me sad. :(

seansfkelley · 2016-11-01T23:17:09Z

+  * @param {String} filters.ignore glob The glob to filter the package name against
+  * @return {Array.<Package>} The packages with a name matching the glob
+  */
+  static getFilteredPackages(packages, {scope, ignore}) {


I would perhaps rename this to make it a bit more obviously different than filterPackages.

getScopedAndIgnoredPackages maybe? :/

I'll have a think of a better name

Or maybe something along the lines of applyFilteringRules or something. Yeah, naming is hard. :(

If filterPackages isn't used elsewhere maybe it should be private like _filterPackages and make this method filterPackages

seansfkelley · 2016-11-01T23:18:38Z

 - `lerna`: the current version of Lerna being used.
 - `version`: the current version of the repository.
 - `publishConfig.ignore`: an array of globs that won't be included in `lerna updated/publish`. Use this to prevent publishing a new version unnecessarily for changes, such as fixing a `README.md` typo.
+- `bootstrapConfig.ignore`: an glob that won't be bootstrapped when running the `lerna bootstrap` command.


Should we mention scope here too?

Ah, I think my original intention was to remove the the scope from bootstrapConfig as it didnt exists previously and I wasnt sure if that was a deliberate choice or not.

Happy to defer to the maintainers though, adding docs for scope here (I have a feeling it would be a rare usecase?) or removing the scope functionality I added here.

I think between (1) functionality consistency and (2) the fact that there is a use-case for "permanent" ignore rules, supporting permanent scope rules is a good idea. Principle of least surprise, and all that.

seansfkelley · 2016-11-01T23:21:12Z

+
+  get packageGraph() {
+    if (!this._packageGraph) {
+      this.buildPackageGraph();


Shouldn't these be called with arguments? I see buildPackageGraph is called in runPreparations, but that seems a bit like spooky action at a distance.

Perhaps Repository should only cache packages/packageGraph and filtering should be performed explicitly by the task that wants to filter (i.e. Command, as it does now).

Hmmm.. I liked the idea of pulling the filtering out of the commands just to make them consistent. Maybe these getters should each take optional { scope, ignore } flags?

Honestly it was done like this to match asini's PR (which originated from here), so I'd be happy for it to be either way

So the big argument against this structure is that these are getters, but depending on the order that you invoke repository.packageGraph and repository.buildPackageGraph(...), you can end up with permanently different values for packageGraph (or one of the other fields controlled by this). Spooky!

There's also a hand-wavier argument about who should be responsible for filtering; I figured that the "Repository" class should probably be a predictable, consistent, immutable representation of the state of the repo, and filtering should be applied elsewhere, namely, in the commands themselves. If you just pulled out the filtering behavior of buildPackageGraph to the root Command you should still get it across commands, yeah?

Lastly, you can't pass arguments to getters!

@lukebatchelor please fix the above before merging!

Cool, ended up doing exactly what you said and moved the responsibility in PackageUtilities and it only gets called in one place still (Command.js).

Much better!

jamiebuilds · 2016-11-16T19:38:47Z

Sorry this was left around, rebase and I'll merge

…strap, exec, run, clean and ls commands

… ignored

…clean and bootstrap commands

… responsibility from Repository

lukebatchelor · 2016-11-20T05:06:58Z

Rebased and refactored. Just hoping Appveyor decides to play nicely today... ...

lukebatchelor · 2016-11-20T05:18:32Z

@seansfkelley Wold you be able to do a quick review of this if you have time?

lukebatchelor · 2016-11-21T12:28:33Z

Ah, has to be an owner. Could you take a look when you're ready @thejameskyle ?

lukebatchelor · 2016-12-01T03:56:25Z

@thejameskyle Just bumping this again. When you're free, cheers!

lukebatchelor · 2016-12-08T04:29:16Z

@thejameskyle I've also updated the follow up to this #390 and that should be ready to go too.

Cheers 🍻

gigabo

This LGTM. It wound up nicer than the Asini version. :)

@lukebatchelor It's fallen out of date with master again. One more rebase?

lukebatchelor · 2016-12-13T09:57:56Z

😎 Should be ready @gigabo

I merged master instead of rebasing, is that okay? Last time I rebased I had to fix each conflict of each commit manually for some reason. Didnt want to have to do that again for this and #390.

(If anyone knows what I might be doing wrong, that would be great too!)

gigabo · 2016-12-13T19:03:12Z

Thanks @lukebatchelor merging should be fine. Going to squash it onto master either way. 👹

FWIW the "fix each conflict of each commit manually" thing is actually a feature of rebasing. It allows the resulting commits to apply cleanly on top of the new base for a linear history. It can be a pain when there are a lot of them, but sometimes I find it's actually easier to see how they should be resolved when they pop up in the context of the individual commits that introduced them.

So, in Asini we followed this change set up with asini/asini#15 and then asini/asini#18. Would you be interested in/willing to port those over here? 😁

lock · 2018-12-27T21:21:40Z

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.

lukebatchelor mentioned this pull request Oct 21, 2016

[Feature] Add --scope to LsCommand to determine which packages are being depended upon #384

Closed

lukebatchelor commented Oct 21, 2016

View reviewed changes

lukebatchelor mentioned this pull request Oct 21, 2016

[Feature] Add a --with-deps flag to bootstrap --scope #388

Closed

lukebatchelor mentioned this pull request Oct 25, 2016

[Feature] Adds --include-filtered-dependencies flag for bootstrap command #390

Merged

seansfkelley reviewed Nov 1, 2016

View reviewed changes

jamiebuilds changed the title ~~Adds --scope and --ignore support for bootstrap, exec, run, clean and ls~~ [Feature] Adds --scope and --ignore support for bootstrap, exec, run, clean and ls Nov 16, 2016

jamiebuilds added the enhancement label Nov 16, 2016

jamiebuilds mentioned this pull request Nov 16, 2016

Support --scope flag in lerna bootstap command #383

Closed

jamiebuilds added enhancement and removed enhancement labels Nov 16, 2016

lukebatchelor and others added 10 commits November 20, 2016 15:18

Refactoring to allow scope and ignore flags to work accross with boot…

3fae761

…strap, exec, run, clean and ls commands

fixed issue with bootstrapConfig and publishCOnfig ignore flags being…

e4f44ac

… ignored

Adds tests for --ignore and --scope flag behaviour in ls, exec, run, …

3fc5a62

…clean and bootstrap commands

Adds tests for PakcageUtilities.getFilteredPackages()

500860f

Linting: removed unused imports

95d9aea

fixes broken test in bootstrap due to non-normalized paths

178caf1

fixes broken tests in BootstrapCommand from windowns symlinks

e97f4d0

Removed the configFlags from PublishCommand

ade196e

Updated readme with --scoped and --ignore flags information

7d7dbcd

fix(): Refactored Repository and PackageUtilities to remove filtering…

f14420a

… responsibility from Repository

lukebatchelor force-pushed the spike-scope-bootsrap branch from d11fff8 to f14420a Compare November 20, 2016 05:01

seansfkelley approved these changes Nov 21, 2016

View reviewed changes

This was referenced Dec 1, 2016

Make bootstrap, exec and run commands execute packages in dependency order by default #412

Merged

[Discussion] Lerna Maintenance #408

Closed

gigabo approved these changes Dec 12, 2016

View reviewed changes

lukebatchelor added 2 commits December 13, 2016 20:31

Merged latest master changes and fixed merge conficts

5d9d8b2

Fixes broken test for LSCommand

d29f08f

gigabo merged commit d707386 into lerna:master Dec 13, 2016

gigabo mentioned this pull request Dec 13, 2016

Highlight private packages in updated/publish output #436

Merged

lock Bot locked as resolved and limited conversation to collaborators Dec 27, 2018


		The `ignore` flag, when used with the `bootstrap` command, can also be set in `lerna.json` under the `bootstrapConfig` key. The command-line flag will take precendence over this option. This flag is supported in `bootstrap` and `exec` commands.

		Note: If both `scope` and `ignore` are provided to `exec` command, `scope` takes precedence.

Conversation

lukebatchelor commented Oct 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukebatchelor commented Oct 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joscha Oct 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukebatchelor commented Oct 23, 2016

Uh oh!

seansfkelley commented Nov 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamiebuilds commented Nov 16, 2016

Uh oh!

lukebatchelor commented Nov 20, 2016

Uh oh!

lukebatchelor commented Nov 20, 2016

Uh oh!

lukebatchelor commented Nov 21, 2016

Uh oh!

lukebatchelor commented Dec 1, 2016

Uh oh!

lukebatchelor commented Dec 8, 2016

Uh oh!

gigabo left a comment

Choose a reason for hiding this comment

Uh oh!

lukebatchelor commented Dec 13, 2016

lukebatchelor commented Oct 21, 2016 •

edited

Loading

joscha Oct 23, 2016 •

edited

Loading