Skip to content

Add style rule for argparse.Namespace access.#3826

Open
ScottTodd wants to merge 1 commit intoROCm:mainfrom
ScottTodd:python-style-argparse-attributes
Open

Add style rule for argparse.Namespace access.#3826
ScottTodd wants to merge 1 commit intoROCm:mainfrom
ScottTodd:python-style-argparse-attributes

Conversation

@ScottTodd
Copy link
Member

Motivation

Review at #3801 (comment) caught some overly defensive coding:

def do_copy(args: argparse.Namespace):
    ...
    source_backend = _create_source_backend(
        source_run_id=args.source_run_id,
        platform=args.platform,
        local_staging_dir=getattr(args, "local_staging_dir", None),  # Here

This adds a style guide rule saying we prefer to trust that args contains attributes:

-       local_staging_dir=getattr(args, "local_staging_dir", None),
+       local_staging_dir=args.local_staging_dir,

Technical Details

Discussion with the agent that authored the change:

> Ok for the review comments, how can we stop weird patterns like `getattr(args, "local_staging_dir", None)` from
  appearing in the first place?

● That's on me — I used getattr defensively because argparse.Namespace is untyped, so there's no static analysis telling
   you the attribute exists. But --local-staging-dir is added by _add_backend_args, which is called on the copy parser,
  so args.local_staging_dir is always present.

  The fix is a style guide rule. The Python style guide already says "explicit over implicit" — this is a case of being
  too defensive. A short note like "trust argparse: use args.foo directly, not getattr(args, 'foo', ...)" would catch
  this in reviews.

  Want me to add that to the Python style guide?

> Ah, and `do_copy(args: argparse.Namespace)` is pretty much untyped... yeah. We have this pattern in a few
  scripts. I'm more a fan of having explicit argument lists instead of passing around `args` everywhere. This is in
   a gray area where each subcommand takes `args`, and the subcommand may then call other functions with specific
  argument lists.

● Right. The do_copy(args: argparse.Namespace) → _create_source_backend(source_run_id, platform, local_staging_dir)
  boundary is actually doing the right thing — do_copy unpacks the bag-of-attributes into typed parameters at the call
  site. The getattr crept in because the untyped args makes you second-guess what's in it.

  The pattern is fine as-is: subcommand entry points take args: Namespace, then unpack into typed calls. The rule is
  just "trust the unpacking — if you defined the arg on that parser, access it directly." No need to refactor the whole
  argparse structure for this.

  I'd add a one-liner to the Python style guide: something like "Access argparse.Namespace attributes directly
  (args.foo), not via getattr. The argument is guaranteed to exist if it was added to the subparser."

Submission Checklist

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.6 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: TODO

Development

Successfully merging this pull request may close these issues.

1 participant