Skip to content

refactor: Unify internal/cmd/util/scope.go parsing into a single Path type with options-driven Parse #259

@tmeckel

Description

@tmeckel

PRIO 1. Strict predecessor of #247. The unified Parse(ctx, raw, opts) (*Path, error) infrastructure must be in place before azdo pipelines agent show can be implemented. This refactor introduces the parse infrastructure only — the new ParsePoolAgentTargetWithDefaultOrganization public wrapper for #247 is added as part of #247's own implementation work, keeping this issue a pure refactor with no new public API.

Why now

internal/cmd/util/scope.go currently exposes 6 near-duplicate parsers, three of which re-implement the same default-org fallback. Every new "target shape" requires either a new struct + parallel parser, or a generalization that pollutes the existing types. A single options-driven Parse function eliminates the duplication and makes the next nested-target case a one-line wrapper that whichever feature needs it can own and add itself — keeping refactors and feature work cleanly separated.

The actual file is only 265 lines, but the duplication makes it hard to read and impossible to extend safely.

Locked Decisions

# Decision Rationale
1 Single canonical type Path { Organization string; Project string; Targets []string } replaces the existing Scope and Target structs. Callers are updated to use *Path. One shape for any input format. Targets []string generalizes naturally to nested targets (length 1 today, length 2 for pool/agent in #247, future length 3 if needed).
2 Single internal function Parse(ctx, raw, opts ParseOptions) (*Path, error) does all the work — splitting, trimming, validation, and default-org resolution. The default-org fallback, segment validation, and whitespace handling live in one place.
3 ParseOptions struct: AllowImplicitOrg bool; RequireProject bool; MinTargets int; MaxTargets int. The 6 existing functions and any future wrapper map to a single Parse(...) call.
4 The 6 existing public function names are preserved (ParseScope, ParseProjectScope, ParseOrganizationArg, ParseTarget, ParseTargetWithDefaultOrganization, ParseProjectTargetWithDefaultOrganization) as thin wrappers that return *Path. Caller call sites need only a target.Targetpath.Targets[0] rename; the function name stays the same so the diff is mechanical.
5 ResolveScopeDescriptor is unchanged (it is not a parser; it is a service-call wrapper that operates on already-parsed scope fields). Orthogonal concern.
6 Variable names at call sites do not need to change (e.g. scope, err := util.ParseProjectScope(ctx, raw) keeps scope as the variable name even though its type changes from *Scope to *Path). Reduces diff noise.
7 All existing scope_test.go cases remain valid with only a result.Targetresult.Targets[0] field rename. New table-driven TestParse covers the same shapes via direct Parse(ctx, raw, opts) calls. Test coverage is preserved and improved.

API Surface

// Path represents a parsed user-input path of the form
//   [ORGANIZATION[/PROJECT]]/TARGET[/SUBTARGET[/...]].
// Organization is always populated after a successful Parse.
type Path struct {
    Organization string
    Project      string
    Targets      []string
}

// ParseOptions configures how a raw user input is split into a Path.
type ParseOptions struct {
    AllowImplicitOrg bool
    RequireProject   bool
    MinTargets       int
    MaxTargets       int
}

// Parse splits a raw user input into a Path according to opts.
// Use the convenience wrappers for the common cases.
func Parse(ctx CmdContext, raw string, opts ParseOptions) (*Path, error)

Convenience wrappers (all return *Path unless noted; all 6 are preserved as thin inline-able wrappers):

Wrapper AllowImplicitOrg RequireProject Min/Max Targets Returns
ParseScope(ctx, raw) true false 0/0 *Path
ParseProjectScope(ctx, raw) true true 0/0 *Path
ParseOrganizationArg(ctx, raw) (via ParseScope + post-check that Project is empty) (string, error)
ParseTarget(raw) false false 1/1 *Path
ParseTargetWithDefaultOrganization(ctx, raw) true false 1/1 *Path
ParseProjectTargetWithDefaultOrganization(ctx, raw) true true 1/1 *Path

Internal implementation sketch

func Parse(ctx CmdContext, raw string, opts ParseOptions) (*Path, error) {
    trimmed := strings.TrimSpace(raw)
    var parts []string
    if trimmed != "" {
        parts = strings.Split(trimmed, "/")
        for i := range parts {
            parts[i] = strings.TrimSpace(parts[i])
            if parts[i] == "" {
                return nil, fmt.Errorf("input %q contains empty segment", raw)
            }
        }
    }

    n := len(parts)
    minExtra := 0
    if opts.RequireProject {
        minExtra = 1
    }
    minOrg := 0
    if !opts.AllowImplicitOrg {
        minOrg = 1
    }
    minTotal := minOrg + minExtra + opts.MinTargets
    maxTotal := 1 + (1 if opts.RequireProject else 0) + opts.MaxTargets

    if n < minTotal || n > maxTotal {
        return nil, fmt.Errorf("invalid input %q: expected %d-%d segments, got %d", raw, minTotal, maxTotal, n)
    }

    p := &Path{Targets: make([]string, opts.MinTargets)}
    copy(p.Targets, parts[n-opts.MinTargets:])

    switch extra := n - opts.MinTargets; extra {
    case 0:
        // both org and project implicit
    case 1:
        if opts.RequireProject {
            p.Project = parts[0]
        } else {
            p.Organization = parts[0]
        }
    case 2:
        p.Organization, p.Project = parts[0], parts[1]
    }

    if p.Organization == "" {
        if ctx == nil {
            return nil, fmt.Errorf("no organization specified and no default organization configured")
        }
        cfg, err := ctx.Config()
        if err != nil {
            return nil, err
        }
        org, err := cfg.Authentication().GetDefaultOrganization()
        if err != nil {
            return nil, fmt.Errorf("no organization specified and no default organization configured: %w", err)
        }
        org = strings.TrimSpace(org)
        if org == "" {
            return nil, fmt.Errorf("no organization specified and no default organization configured")
        }
        p.Organization = org
    }

    return p, nil
}

Migration impact

  • target.Targetpath.Targets[0] at ~20 call sites.
  • scope.Organization / scope.Project field access unchanged (names match on both *Scope and *Path).
  • All 6 existing public functions keep their names; only their return types change from *Scope/*Target to *Path (silent change at call sites that use field access).
  • Files touched: ~35 (all callers + internal/cmd/util/scope.go + internal/cmd/util/scope_test.go).
  • ResolveScopeDescriptor callers: no change.
  • No new public API is introduced by this refactor. Future wrappers (e.g. ParsePoolAgentTargetWithDefaultOrganization for feat: Implement azdo pipelines agent show command #247) are added by whichever feature needs them, as thin wrappers over Parse.

Test plan

  • All existing scope_test.go cases pass after the result.Targetresult.Targets[0] rename.
  • New TestParse table-driven test: every (input, options) combination from the 6 existing wrappers, expressed as direct Parse(ctx, raw, opts) calls. Drives the same code paths the wrappers use.
  • New edge cases for Parse: empty input rejected when !AllowImplicitOrg; whitespace-only input; segments with only whitespace; ctx == nil when default org is needed.

Tooling and Verification

  • gofmt / gofumpt on touched files
  • go test ./internal/cmd/util/...
  • go test ./...
  • make lint

Blocks

Reference

Metadata

Metadata

Assignees

Labels

P1Priority 1clienhancementNew feature or request

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions