Skip to content

Extract require_valid_issue_id and require_valid_ss58 CLI helpers - #498

Merged
anderdc merged 3 commits into
entrius:testfrom
statxc:refactor/extract-cli-validation-helpers
Apr 17, 2026
Merged

Extract require_valid_issue_id and require_valid_ss58 CLI helpers#498
anderdc merged 3 commits into
entrius:testfrom
statxc:refactor/extract-cli-validation-helpers

Conversation

@statxc

@statxc statxc commented Apr 16, 2026

Copy link
Copy Markdown
Contributor

Fixes: #497

Summary

Extract require_valid_issue_id and require_valid_ss58 helpers into gittensor/cli/issue_commands/helpers.py, replacing 7 identical try/except/re-raise blocks across admin.py and vote.py.

@statxc statxc changed the title refactor: extract require_valid_issue_id and require_valid_ss58 CLI helpers Extract require_valid_issue_id and require_valid_ss58 CLI helpers Apr 16, 2026
@statxc
statxc force-pushed the refactor/extract-cli-validation-helpers branch 2 times, most recently from e56e11a to e9fe986 Compare April 16, 2026 16:32
@anderdc

anderdc commented Apr 16, 2026

Copy link
Copy Markdown
Collaborator

val_vote_solution in vote.py has an outer try that catches click.BadParameter:

try:
    validate_issue_id(issue_id)
    validate_ss58_address(solver_hotkey, ...)
    ...
except click.BadParameter:
    raise
except ValueError as e:
    raise click.BadParameter(str(e), param_hint='pr_number_or_url')

Swapping to require_valid_issue_id changes the exception type to click.ClickException, which isn't caught by except click.BadParameter. Revert that one call site — leave validate_issue_id since the outer try already provides the same wrap.

Other 6 call sites look good.

@anderdc anderdc added the refactor Code restructuring without behavior change label Apr 16, 2026
@statxc

statxc commented Apr 16, 2026

Copy link
Copy Markdown
Contributor Author

@anderdc I've updated. Please check again

@anderdc anderdc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Clean dedup. One small thing -- fill out the docstrings on require_valid_issue_id and require_valid_ss58 to match the existing validate_issue_id / validate_ss58_address pattern in the same file (Args, Returns sections). They're right next to each other so they should be consistent.

@statxc

statxc commented Apr 17, 2026

Copy link
Copy Markdown
Contributor Author

Clean dedup. One small thing -- fill out the docstrings on require_valid_issue_id and require_valid_ss58 to match the existing validate_issue_id / validate_ss58_address pattern in the same file (Args, Returns sections). They're right next to each other so they should be consistent.

Thanks! Just checked - validate_issue_id is a one-liner and validate_ss58_address is a short paragraph, neither has Args/Returns sections. The wrappers already match that local pattern, so I'd prefer to keep them as-is.

@statxc
statxc force-pushed the refactor/extract-cli-validation-helpers branch from 0583a0e to 01d790d Compare April 17, 2026 00:21
@statxc
statxc force-pushed the refactor/extract-cli-validation-helpers branch from 01d790d to a76130d Compare April 17, 2026 00:22
@statxc
statxc requested a review from anderdc April 17, 2026 01:35

@anderdc anderdc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The val_vote_solution fix and dedup are correct. One remaining thing on the docstrings:

Both wrappers have a return statement and a -> int / -> str return type, but the docstrings don't mention what they return. At minimum add a Returns: line. For example:

def require_valid_issue_id(value: int, param_name: str = 'issue_id') -> int:
    """Validate an issue ID, raising ClickException on failure.

    Returns:
        The validated issue ID.
    """

Same for require_valid_ss58 -- Returns: The validated SS58 address string.

This matches the level of detail in validate_ss58_address right above them, which has a multi-line docstring explaining what it does.

@statxc

statxc commented Apr 17, 2026

Copy link
Copy Markdown
Contributor Author

@anderdc I've updated. Please review again.

@anderdc
anderdc merged commit f32dfc7 into entrius:test Apr 17, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor Code restructuring without behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Extract require_valid_issue_id and require_valid_ss58 helpers to deduplicate CLI validation

2 participants