Skip to content

feat(version-scanner): support target list inputs and refine python version checks#17475

Closed
chalmerlowe wants to merge 1 commit into
mainfrom
feat/version-scanner-target-list
Closed

feat(version-scanner): support target list inputs and refine python version checks#17475
chalmerlowe wants to merge 1 commit into
mainfrom
feat/version-scanner-target-list

Conversation

@chalmerlowe

@chalmerlowe chalmerlowe commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

This pull request enables scanning for multiple dependency and version targets (e.g. python [3.7, 3.8, 3.9]) in a single scan, easing monorepo compatibility testing.

Additional Key changes:

  • Adds a unified --targets option to parse YAML/JSON files or inline strings.
  • Polishes CSV and Sheet outputs to split file path into directory/filename columns and display dependencies upfront, making it easier to do analysis of problematic files regardless of package.
  • Updates python-specific rules in regex_config.yaml to cover subscript minor checks (sys.version_info[1]) and optional patch numbers.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request enhances the version scanner script to support scanning for multiple dependency and version targets simultaneously. It introduces a new --targets CLI argument that accepts a file path or inline YAML/JSON string, adds a new dependency_requirement rule, and updates reporting to include file names, dependencies, and versions. The accompanying unit tests have been updated and expanded to cover these new features. The review feedback suggests several robustness improvements: explicitly handling non-existent file paths to avoid misleading YAML/JSON parsing errors, filtering out None values when dependencies are defined without versions, and tightening CLI argument validation to prevent invalid combinations of --targets and individual target options.

Comment on lines +605 to +614
if os.path.exists(targets_input):
try:
with open(targets_input, 'r', encoding='utf-8') as f:
content = f.read()
except PermissionError:
print(f"Error: Permission denied reading targets file: {targets_input}", file=sys.stderr)
sys.exit(1)
except Exception as e:
print(f"Error reading targets file {targets_input}: {e}", file=sys.stderr)
sys.exit(1)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If the targets_input is a file path that does not exist, os.path.exists returns False, causing the scanner to treat the filename as an inline YAML/JSON string. This leads to a misleading error message: Error: Targets input must resolve to a JSON object or YAML mapping. We should check if the input looks like a file path and explicitly report if it is not found.

Suggested change
if os.path.exists(targets_input):
try:
with open(targets_input, 'r', encoding='utf-8') as f:
content = f.read()
except PermissionError:
print(f"Error: Permission denied reading targets file: {targets_input}", file=sys.stderr)
sys.exit(1)
except Exception as e:
print(f"Error reading targets file {targets_input}: {e}", file=sys.stderr)
sys.exit(1)
looks_like_file = not ("{" in targets_input or "\n" in targets_input) and (
os.path.splitext(targets_input)[1] in [".yaml", ".yml", ".json"] or not ":" in targets_input
)
if looks_like_file and not os.path.exists(targets_input):
print(f"Error: Targets file not found: {targets_input}", file=sys.stderr)
sys.exit(1)
if os.path.exists(targets_input):
try:
with open(targets_input, 'r', encoding='utf-8') as f:
content = f.read()
except PermissionError:
print(f"Error: Permission denied reading targets file: {targets_input}", file=sys.stderr)
sys.exit(1)
except Exception as e:
print(f"Error reading targets file {targets_input}: {e}", file=sys.stderr)
sys.exit(1)

Comment on lines +626 to +632
targets = []
for dep, versions in raw_targets.items():
if isinstance(versions, list):
for v in versions:
targets.append((str(dep), str(v)))
else:
targets.append((str(dep), str(versions)))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If a dependency in the targets YAML/JSON is defined without a version (e.g., python: with no value, which parses as None), the scanner will append ("python", "None") and scan for the literal string "None". We should handle None values to avoid unexpected scanning behavior.

Suggested change
targets = []
for dep, versions in raw_targets.items():
if isinstance(versions, list):
for v in versions:
targets.append((str(dep), str(v)))
else:
targets.append((str(dep), str(versions)))
targets = []
for dep, versions in raw_targets.items():
if versions is None:
print(f"Warning: No versions specified for dependency '{dep}'. Skipping.", file=sys.stderr)
continue
if isinstance(versions, list):
for v in versions:
if v is not None:
targets.append((str(dep), str(v)))
else:
targets.append((str(dep), str(versions)))

Comment on lines +724 to +730
has_single_target = bool(args.dependency and args.version)
has_targets_list = bool(args.targets)

if not (has_single_target or has_targets_list):
parser.error("Must specify either (-d/--dependency AND -v/--version) OR (--targets)")
if has_single_target and has_targets_list:
parser.error("Cannot specify both single target (-d/-v) and targets list (--targets)")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The current validation logic allows invalid combinations such as specifying --targets along with only -d (without -v), which silently ignores -d. We should explicitly disallow any individual target options (-d or -v) when --targets is used, and require both -d and -v when --targets is not used.

Suggested change
has_single_target = bool(args.dependency and args.version)
has_targets_list = bool(args.targets)
if not (has_single_target or has_targets_list):
parser.error("Must specify either (-d/--dependency AND -v/--version) OR (--targets)")
if has_single_target and has_targets_list:
parser.error("Cannot specify both single target (-d/-v) and targets list (--targets)")
if args.targets:
if args.dependency or args.version:
parser.error("Cannot specify both targets list (--targets) and individual target options (-d/-v)")
else:
if not (args.dependency and args.version):
parser.error("Must specify both -d/--dependency and -v/--version when not using --targets")

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant