Skip to content

tech-debt: make option-heavy public signatures keyword-only #1372

Description

@ogenstad

Component

Python SDK

Need

The SDK's most-used client methods accept a long tail of optional arguments positionally. filters has 19 positional-capable parameters, all 18 and get 17 (each roughly doubled across the async and sync clients), and 25 public callables sit at 8 or more.

Only the leading argument is genuinely positional in practice - kind for the client query methods - and everything after it is an option. Because those options are positionally bindable, any change to their order or any insertion between them is a silent breaking change: existing callers keep working but rebind a value to a different parameter, with no error and no type-checker signal.

This is not hypothetical. During review of #1120 a new merge option was added to get, all and filters between populate_store and fragment, and a review bot correctly pointed out that a caller passing fragment or anything after it positionally would now bind it to merge instead. We judged the risk acceptable for that PR on the grounds that nobody realistically passes eleven positional arguments, and left the parameter in its logical position next to populate_store. That reasoning holds for one change, but it does not remove the hazard - it just leaves it in place for the next person to add an option.

Enforcing keyword-only arguments after the leading required one would make this class of mistake impossible and let options be grouped by meaning rather than by arrival order.

Use case

Two audiences benefit:

  • SDK maintainers gain the freedom to add, reorder or group options without auditing for positional callers, and lose an entire category of silent breakage in review.
  • SDK users get an immediate TypeError instead of a value quietly landing in the wrong parameter. Call sites also become self-describing, which matters most in the network-automation scripts and generators this SDK is written for, where a misbound boolean can change what is written to the source of truth.

Additional information

The change is breaking and needs a major release. The break is shallow - it only affects callers already passing options positionally, which is the same population the merge discussion was about - but it is a real signature change and should be called out in the changelog and upgrade notes.

Worth deciding as part of the work:

  • Where the keyword-only boundary sits for each callable (for the client query methods, most likely immediately after kind, with raise_when_missing a judgement call).
  • Whether to cover infrahubctl command functions and internal helpers, or restrict the change to the documented public API.
  • Whether a deprecation period is feasible ahead of the major, for example warning on positional use before enforcing it.

The 25 affected callables can be regenerated by walking the AST of infrahub_sdk/ for public functions whose positional-capable parameter count crosses a threshold; the count above used 8.

Raised during review of #1120.


Assisted-by: opsmill-dev-creating-issues 0.2.0

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    effort/mediumThis issue should be completed in a less than a daystate/need-triageThis issue needs to be triagedtype/tech-debtItem we know we need to improve way it is implemented

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions