Skip to content

Implement Role Marker and Wrapper Function for Github Cli#266

Open
desmondwong1215 wants to merge 17 commits intogit-mastery:mainfrom
desmondwong1215:infra/tour9-10-autograding
Open

Implement Role Marker and Wrapper Function for Github Cli#266
desmondwong1215 wants to merge 17 commits intogit-mastery:mainfrom
desmondwong1215:infra/tour9-10-autograding

Conversation

@desmondwong1215
Copy link
Copy Markdown
Contributor

@desmondwong1215 desmondwong1215 commented Feb 24, 2026

Add Role Marker.
Add helper function to add config to gitmastery-exercise.json.
Add wrapper function for GitHub CLI.

@github-actions
Copy link
Copy Markdown

Hi @desmondwong1215, thank you for your contribution! 🎉

This PR comes from your fork desmondwong1215/exercises on branch infra/tour9-10-autograding.

Before you request for a review, please ensure that you have tested your changes locally!

Important

The previously recommended way of using ./test-download.py is no longer the best way to test your changes locally.

Please read the following instructions for the latest instructions.

Prerequisites

Ensure that you have the gitmastery app installed locally (instructions)

Testing steps

If you already have a local Git-Mastery root to test, you can skip the following step.

Create a Git-Mastery root locally:

gitmastery setup

Navigate into the Git-Mastery root (defaults to gitmastery-exercises/):

cd gitmastery-exercises/

Edit the .gitmastery.json configuration file. You need to set the following values under the exercises_source key.

{
    # other fields...
    "exercises_source": {
        "username": "desmondwong1215",
        "repository": "exercises",
        "branch": "infra/tour9-10-autograding"
    }
}

Then, you can use the gitmastery app to download and verify your changes locally.

gitmastery download <your new change>
gitmastery verify

Checklist

  • (For exercises and hands-ons) I have verified that the downloading behavior works
  • (For exercises only) I have verified that the verification behavior is accurate

Important

To any reviewers of this pull request, please use the same instructions above to test the changes.

@desmondwong1215 desmondwong1215 changed the title Implement infrastructure for Tour 9 and 10 autograding Implement infrastructure for autograding Tour 9 and 10 Feb 24, 2026
@desmondwong1215 desmondwong1215 changed the title Implement infrastructure for autograding Tour 9 and 10 Implement Role Marker and Wrapper Function for Github Cli Feb 25, 2026
@VikramGoyal23 VikramGoyal23 self-requested a review February 27, 2026 09:46
@desmondwong1215 desmondwong1215 marked this pull request as draft March 10, 2026 03:00
@desmondwong1215 desmondwong1215 force-pushed the infra/tour9-10-autograding branch from cebe009 to b6eee08 Compare March 10, 2026 05:01
@desmondwong1215 desmondwong1215 marked this pull request as ready for review March 10, 2026 05:03
Comment on lines +42 to +43
def add_pr_config(pr_number: int, pr_repo_full_name: str) -> None:
update_config_fields(
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.

A short docstring here would be nice, something like:

Suggested change
def add_pr_config(pr_number: int, pr_repo_full_name: str) -> None:
update_config_fields(
def add_pr_config(pr_number: int, pr_repo_full_name: str) -> None:
"""Adds a PR config to .gitmastery-exercise.json."""
update_config_fields(

Comment on lines +254 to +261
command = _build_pr_command(
"list",
"--state",
validated_state,
"--json",
"number,title,state,author,headRefName,baseRefName",
repo_name=repo_name,
)
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.

Keep in mind that doing it this way only lists the 30 most recent PRs. To list more/less of them, I recommend adding the --limit flag with a limit parameter which allows you to manually define the number of PRs to list.

Comment on lines +248 to +268
def list_prs(state: str, repo_name: str, verbose: bool) -> list[dict[str, Any]]:
"""
List pull requests.
PR state filter ('open', 'closed', 'merged', 'all')
"""
validated_state = _validate_choice(state, _PR_STATES, "state")
command = _build_pr_command(
"list",
"--state",
validated_state,
"--json",
"number,title,state,author,headRefName,baseRefName",
repo_name=repo_name,
)

result = run(command, verbose)

if result.is_success():
parsed = _parse_json_or_default(result.stdout, [])
return parsed if isinstance(parsed, list) else []
return []
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.

I highly recommend adding support for the --search flag since I foresee it being useful in the future.

repo_name: str,
verbose: bool,
) -> bool:
"""Submit a review on a pull request with automatic role marker. True if review was submitted successfully, False otherwise"""
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.

Incorrect docstring formatting. Considering changing it to:

Suggested change
"""Submit a review on a pull request with automatic role marker. True if review was submitted successfully, False otherwise"""
"""
Submit a review on a pull request with automatic role marker.
True if review was submitted successfully, False otherwise
"""

Comment on lines +24 to +25
"""Format text with a role marker.
Example:
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.

Formatting nit:

Suggested change
"""Format text with a role marker.
Example:
"""
Format text with a role marker.
Example:

Comment on lines +8 to +9
"""Wrapper for git and GitHub operations with automatic role marker formatting.

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.

Formatting nit:

Suggested change
"""Wrapper for git and GitHub operations with automatic role marker formatting.
"""
Wrapper for git and GitHub operations with automatic role marker formatting.

Copy link
Copy Markdown
Collaborator

@VikramGoyal23 VikramGoyal23 left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, just some nits regarding role helpers.

Comment on lines +75 to +85
verbose: bool,
) -> Optional[int]:
"""Create a pull request with automatic role markers."""
return github_cli.create_pr(
self._format_text(title),
self._format_text(body),
base,
head,
repo_name,
verbose,
)
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.

Developers are unable to create draft PRs with role markers since this function doesn't allow passing in a draft parameter.

Suggested change
verbose: bool,
) -> Optional[int]:
"""Create a pull request with automatic role markers."""
return github_cli.create_pr(
self._format_text(title),
self._format_text(body),
base,
head,
repo_name,
verbose,
)
verbose: bool,
draft: bool = False,
) -> Optional[int]:
"""Create a pull request with automatic role markers."""
return github_cli.create_pr(
self._format_text(title),
self._format_text(body),
base,
head,
repo_name,
verbose,
draft,
)

Comment on lines +123 to +128
comment: Optional[str] = None,
verbose: bool = False,
) -> bool:
"""Close a pull request without merging."""
formatted_comment = self._format_text(comment) if comment else None
return github_cli.close_pr(pr_number, repo_name, formatted_comment, verbose)
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.

This is missing the delete_branch parameter. Running this command will add the delete-branch flag according to the value of verbose.

Suggested change
comment: Optional[str] = None,
verbose: bool = False,
) -> bool:
"""Close a pull request without merging."""
formatted_comment = self._format_text(comment) if comment else None
return github_cli.close_pr(pr_number, repo_name, formatted_comment, verbose)
comment: Optional[str] = None,
delete_branch: bool = False,
verbose: bool = False,
) -> bool:
"""Close a pull request without merging."""
formatted_comment = self._format_text(comment) if comment else None
return github_cli.close_pr(
pr_number,
repo_name,
formatted_comment,
delete_branch,
verbose
)

Copy link
Copy Markdown
Collaborator

@VikramGoyal23 VikramGoyal23 left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment on lines +14 to +15
if not value:
continue
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.

Are you sure about this change? What this effectively does is prevent you from putting Falsy values as updates to values (e.g. False, "", []). While you most likely do not want to update a config with Falsy values, all this really does is add a bit of inflexibility to the use of the function.

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.

2 participants