-
Notifications
You must be signed in to change notification settings - Fork 168
aitools: add --scope flag, deprecate --project/--global #5234
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,6 @@ | ||
| package aitools | ||
|
|
||
| import ( | ||
| "errors" | ||
| "fmt" | ||
| "maps" | ||
| "slices" | ||
|
|
@@ -19,29 +18,35 @@ import ( | |
| var listSkillsFn = defaultListSkills | ||
|
|
||
| func NewListCmd() *cobra.Command { | ||
| var scopeFlag string | ||
| var projectFlag, globalFlag bool | ||
|
|
||
| cmd := &cobra.Command{ | ||
| Use: "list", | ||
| Short: "List installed AI tools components", | ||
| Args: cobra.NoArgs, | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| if projectFlag && globalFlag { | ||
| return errors.New("cannot use --global and --project together") | ||
| projectFlag, globalFlag, err := parseScopeFlag(scopeFlag, projectFlag, globalFlag, true) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // For list: no flag = show both scopes (empty string). | ||
|
|
||
| // list: empty scope = show both. Both flags set is equivalent. | ||
| var scope string | ||
| if projectFlag { | ||
| switch { | ||
| case projectFlag && !globalFlag: | ||
| scope = installer.ScopeProject | ||
| } else if globalFlag { | ||
| case globalFlag && !projectFlag: | ||
| scope = installer.ScopeGlobal | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should-fix: this changes |
||
| return listSkillsFn(cmd, scope) | ||
| }, | ||
| } | ||
|
|
||
| cmd.Flags().StringVar(&scopeFlag, "scope", "", "Scope to show: project, global, or both (default: both)") | ||
| cmd.Flags().BoolVar(&projectFlag, "project", false, "Show only project-scoped skills") | ||
| cmd.Flags().BoolVar(&globalFlag, "global", false, "Show only globally-scoped skills") | ||
| markScopeBoolsDeprecated(cmd) | ||
| return cmd | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ import ( | |
| "github.com/databricks/cli/libs/aitools/installer" | ||
| "github.com/databricks/cli/libs/cmdio" | ||
| "github.com/databricks/cli/libs/env" | ||
| "github.com/spf13/cobra" | ||
| ) | ||
|
|
||
| // promptScopeSelection is a package-level var so tests can replace it with a mock. | ||
|
|
@@ -82,6 +83,55 @@ func defaultPromptScopeSelection(ctx context.Context) (string, error) { | |
|
|
||
| const scopeBoth = "both" | ||
|
|
||
| // markScopeBoolsDeprecated hides --project and --global from help and emits a | ||
| // stderr warning pointing at --scope when they're used. The booleans are kept | ||
| // so existing scripts and the experimental backward-compat aliases keep | ||
| // working through the next release. | ||
| func markScopeBoolsDeprecated(cmd *cobra.Command) { | ||
| cmd.Flags().Lookup("project").Deprecated = "use --scope=project" | ||
| cmd.Flags().Lookup("project").Hidden = true | ||
| cmd.Flags().Lookup("global").Deprecated = "use --scope=global" | ||
| cmd.Flags().Lookup("global").Hidden = true | ||
| } | ||
|
Comment on lines
+86
to
+95
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this hides |
||
|
|
||
| // parseScopeFlag translates --scope into the equivalent --project/--global bool pair. | ||
| // Returns (projectFlag, globalFlag, nil) unchanged when --scope is empty so the | ||
| // deprecated booleans can keep flowing through the existing resolveScope* helpers. | ||
| // Errors if --scope is combined with --project or --global, or if both deprecated | ||
| // flags are set together (matching the pre-refactor validation). When allowBoth is | ||
| // false, --scope=both is rejected up front so install and uninstall don't have | ||
| // to special-case it. | ||
| func parseScopeFlag(scopeFlag string, projectFlag, globalFlag, allowBoth bool) (proj, glob bool, err error) { | ||
| if scopeFlag == "" { | ||
| // Preserve the pre-refactor behavior: combining the two deprecated flags | ||
| // is always wrong, regardless of allowBoth. Users who want both scopes | ||
| // should use --scope=both (where supported). | ||
| if projectFlag && globalFlag { | ||
| return false, false, errors.New("cannot use --global and --project together") | ||
| } | ||
| return projectFlag, globalFlag, nil | ||
|
Comment on lines
+109
to
+110
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should-fix: this creates a new regression for |
||
| } | ||
| if projectFlag || globalFlag { | ||
| return false, false, errors.New("cannot use --scope with --project or --global; --project and --global are deprecated aliases for --scope") | ||
| } | ||
| switch scopeFlag { | ||
| case installer.ScopeProject: | ||
| return true, false, nil | ||
| case installer.ScopeGlobal: | ||
| return false, true, nil | ||
| case scopeBoth: | ||
| if !allowBoth { | ||
| return false, false, errors.New("--scope=both is not supported for this command; use 'project' or 'global'") | ||
| } | ||
| return true, true, nil | ||
| default: | ||
| if allowBoth { | ||
| return false, false, fmt.Errorf("invalid --scope %q: must be one of project, global, both", scopeFlag) | ||
| } | ||
| return false, false, fmt.Errorf("invalid --scope %q: must be one of project, global", scopeFlag) | ||
| } | ||
| } | ||
|
|
||
| // detectInstalledScopes checks which scopes have a .state.json file present. | ||
| func detectInstalledScopes(globalDir, projectDir string) (global, project bool, err error) { | ||
| globalState, err := installer.LoadState(globalDir) | ||
|
|
||
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.
Nit: this comment is stale after the fixup. Both deprecated flags are now rejected by
parseScopeFlag; the "both" behavior is only the empty/default scope or explicit--scope=both.