Options cleanup #1295
Options cleanup #1295
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
cli/internal/scope/scope_test.go
Outdated
| Since: tc.since, | ||
| IgnorePatterns: []string{tc.ignore}, | ||
| GlobalDepPatterns: tc.globalDeps, | ||
| IncludeDependencies: tc.includeDependencies, | ||
| IncludeDependents: tc.includeDependents, | ||
| }, scm, &context.Context{ | ||
| }, "dummy-repo-root", scm, &context.Context{ |
There was a problem hiding this comment.
Change request: can you make this look more like a path? I had to go check what was going on.
ResolvePackages is well-suited for fs.AbsolutePath with only a definition, invocation, and tests.
There was a problem hiding this comment.
Changed to filepath.FromSlash("/dummy/repo/root").
Currently, the path passed in to ResolvePackages is used by a bunch of the filter code, so while I agree it needs to eventually be an AbsolutePath, it's a non-trivial change.
cli/internal/run/run.go
Outdated
| @@ -197,7 +197,7 @@ func (c *RunCommand) Run(args []string) int { | |||
| return 1 | |||
| } | |||
| } | |||
| filteredPkgs, isAllPackages, err := scope.ResolvePackages(runOptions.scopeOpts(), scmInstance, ctx, c.Ui, c.Config.Logger) | |||
| filteredPkgs, isAllPackages, err := scope.ResolvePackages(&runOptions.scopeOpts, c.Config.Cwd.ToStringDuringMigration(), scmInstance, ctx, c.Ui, c.Config.Logger) | |||
There was a problem hiding this comment.
That this is called cwd throws me for a loop. I want to split "directory in which the command was invoked" from "directory which is being treated as the execution root" and cwd is woefully incomplete at expressing that.
Shouldn't be handled in this PR, but as you're thinking about things, this is something to keep in mind.
There was a problem hiding this comment.
Agreed. I think a rename of the config field is in order at some point.
| FilterPatterns []string | ||
| // IncludeDependents is whether to include dependent impacted consumers in execution (defaults to true) | ||
| IncludeDependents bool | ||
| // Entrypoints is a list of package entrypoints |
There was a problem hiding this comment.
DAG entrypoints, soon enough. (not a change request)
cli/internal/run/run.go
Outdated
| @@ -476,7 +474,7 @@ func parseRunArgs(args []string, config *config.Config, output cli.Ui) (*RunOpti | |||
| runOptions.profile = fmt.Sprintf("%v-profile.json", time.Now().UnixNano()) | |||
|
|
|||
| case strings.HasPrefix(arg, "--no-deps"): | |||
| runOptions.scopeOpts.IncludeDependents = false | |||
| runOptions.scopeOpts.SkipDependents = true | |||
There was a problem hiding this comment.
It even matches our command line flag better! 👍
| @@ -79,7 +85,9 @@ func ResolvePackages(opts *Opts, cwd string, scm scm.SCM, ctx *context.Context, | |||
| Cwd: cwd, | |||
| PackagesChangedSince: opts.getPackageChangeFunc(scm, cwd, ctx.PackageInfos), | |||
| } | |||
| filterPatterns := opts.asFilterPatterns() | |||
| filterPatterns := opts.FilterPatterns | |||
| legacyFilterPatterns := opts.LegacyFilter.asFilterPatterns() | |||
There was a problem hiding this comment.
Nice to tuck these away for easier future removal.
| @@ -225,7 +225,7 @@ func (c *RunCommand) Run(args []string) int { | |||
| rs := &runSpec{ | |||
| Targets: targets, | |||
| FilteredPkgs: filteredPkgs, | |||
| Opts: runOptions, | |||
| Opts: opts, | |||
There was a problem hiding this comment.
Our option boundaries between c (RunCommand) and rs (RunSpec) here are a little weird to me. Just noting, not a request for change.
There was a problem hiding this comment.
RunCommand is not long for this world. The next PR in this series turns it into a wrapper around a cobra command.
| profile: "", // empty string does no tracing | ||
| only: false, | ||
|
|
||
| func getDefaultOptions(config *config.Config) *Opts { |
There was a problem hiding this comment.
This is a nice incremental step.
Cleans up our
RunOptionsin advance of porting over tocobra. Attempts to have the zero value be the default in as many cases as possible, which means that some booleans needed to be flipped to be expressed internally as default: false. The external-facing arguments are unchanged.Going commit-by-commit is probably the best way to go through this one.
There's definitely some refactoring still to be done around
runandrunOpts, but this felt like a big enough change to stand on its own.