Implement exercise T4L2/tags-push #95

VikramGoyal23 · 2025-10-28T19:03:06Z

Exercise Review

Exercise Discussion

#63

Checklist

If you require a new remote repository on the Git-Mastery organization, have you created a request for it?
Have you written unit tests using repo-smith to validate the exercise grading scheme?
Have you tested the download script using test-download.sh?
Have you verified that this exercise does not already exist or is not currently in review?
Did you introduce a new grading mechanism that should belong to git-autograder?
Did you introduce a new dependency that should belong to app?

VikramGoyal23 · 2025-10-28T19:05:05Z

The tests are implemented entirely through mocking due to the unique nature of the exercise.

VikramGoyal23 · 2025-10-29T06:18:04Z

Fixes #63

woojiahao

Left some comments, but overall, the exercise structure makes sense!

woojiahao · 2025-11-20T09:31:49Z

tags_push/README.md

Could you add the instructions for students to this README?

woojiahao · 2025-11-20T09:32:10Z

tags_push/download.py

+TAG_2_MESSAGE = "First stable roster"
+
+def setup(verbose: bool = False):
+    create_start_tag(verbose)


I don't think we need this start tag!

woojiahao · 2025-11-20T09:33:00Z

tags_push/download.py

+    create_start_tag(verbose)
+    run_command(["git", "remote", "rename", "origin", REMOTE_NAME], verbose)
+    tag(TAG_DELETE_NAME, verbose)
+    push(REMOTE_NAME, "--tags", verbose) # somewhat hacky, maybe use run_command instead


Yeap this is a hack, please use run directly! I would avoid undocumented hacks to save future confusion

woojiahao · 2025-11-20T09:36:25Z

tags_push/download.py

+REMOTE_NAME = "production"
+TAG_1_NAME = "v1.0"
+TAG_2_NAME = "v2.0"
+TAG_DELETE_NAME = "beta"
+TAG_2_MESSAGE = "First stable roster"


I would be wary of including so many constants that are only used once. It's easier if you inline these values directly

woojiahao · 2025-11-20T09:36:44Z

tags_push/download.py

+from exercise_utils.git import tag, push
+from exercise_utils.gitmastery import create_start_tag
+
+__resources__ = {}


Suggested change

__resources__ = {}

Since we don't need any resources, remove this line!

woojiahao · 2025-11-20T09:44:46Z

tags_push/verify.py

+    username = get_username()
+    if username is None:
+        raise exercise.wrong_answer([IMPROPER_GH_CLI_SETUP])


In all fairness, this should not be necessary because the requires_github check already covers it, but I get the current constraint. Happy to keep this for now.

woojiahao · 2025-11-20T09:45:47Z

tags_push/tests/test_verify.py

+        pytest.raises(GitAutograderWrongAnswerException) as exception,
+    ):
+        verify(exercise)


I believe that you can use pytest.raises(Exception, match='') to match the body of the exception! So you shouldn't need to run a separate assert

woojiahao · 2025-11-20T09:45:54Z

tags_push/tests/test_verify.py

+        pytest.raises(GitAutograderWrongAnswerException) as exception,
+    ):
+        verify(exercise)


See previous comment

woojiahao · 2025-11-20T09:46:30Z

tags_push/tests/test_verify.py

+        pytest.raises(GitAutograderWrongAnswerException) as exception,
+    ):
+        verify(exercise)
+
+    assert exception.value.message == [IMPROPER_GH_CLI_SETUP]


See previous comment

woojiahao · 2025-11-20T09:47:41Z

tags_push/tests/test_verify.py

It would be good to include the case where all checks fail so we're verifying against multiple comments.

VikramGoyal23 · 2025-11-21T06:01:38Z

Thanks for the feedback. I should hopefully resolve these issues within the next 1-2 days.

- Added instructions to README - Removed hacks and cleaned up download.py - Removed GH API call - Cleaned up test_verify.py - Added new test

VikramGoyal23 · 2025-11-24T10:53:36Z

@woojiahao Could you review my changes?

woojiahao

Minor nit for formatting. The change I've suggested might affect the unit tests, so it will be good to verify that.

Could you re-run the ./test-download.sh tags-push script to ensure that all the tags have the right message?

woojiahao · 2025-11-27T07:55:10Z

tags_push/download.py

-    run_command(["git", "tag", TAG_1_NAME, "HEAD~4"], verbose)
-    run_command(["git", "tag", "-a", TAG_2_NAME, "HEAD~1", "-m", f"\"{TAG_2_MESSAGE}\""], verbose)
+    run_command(["git", "tag", "v1.0", "HEAD~4"], verbose)
+    run_command(["git", "tag", "-a", "v2.0", "HEAD~1", "-m", f"\"{"First stable roster"}\""], verbose)


Suggested change

run_command(["git", "tag", "-a", "v2.0", "HEAD~1", "-m", f"\"{"First stable roster"}\""], verbose)

run_command(["git", "tag", "-a", "v2.0", "HEAD~1", "-m", "First stable roster"], verbose)

The quotes in the code from the exercise review were for the -m parameter, it's not necessary when passed this way!

VikramGoyal23 · 2025-12-02T05:34:26Z

I've fixed the formatting issue and done the necessary checks- the unit tests pass and the tags have the correct message.

jovnc

LGTM, just some minor nits on coding standard and abstraction. The verify logic seems right to me.

jovnc · 2025-12-04T01:39:37Z

tags_push/tests/test_verify.py

+    ):
+        output = verify(exercise)
+        assert_output(output, GitAutograderStatus.SUCCESSFUL)
+


If I'm not wrong, it seems like coding standard in this repository is 2 new lines between each method. This is based on my observation of other files within this codebase.

You're right, let me fix that.

jovnc · 2025-12-04T01:46:38Z

tags_push/verify.py

+TAG_2_MISSING = f"Tag {TAG_2_NAME} is missing, did you push it to the remote?"
+TAG_DELETE_NOT_REMOVED = f"Tag {TAG_DELETE_NAME} is still on the remote!"
+
+def run_command(command: List[str]) -> Optional[str]:


There is a run_command and run_command_with_no_exit util under exercise_util, do you think we can reuse those instead? Or is there something specific in this function that makes this function necessary?

There seems to be a convention to not import anything from exercise_utils for the autograding (See fork_repo and clone_repo) and to define the functions in the verify namespace. Thus, I follow a similar convention.

jovnc · 2025-12-04T01:46:55Z

tags_push/verify.py

+        return None
+
+
+def get_username() -> Optional[str]:


get_username and get_remote_tags seems to be reusable components that can be placed under exercise_utils, do you think we should move it there?

See reply to run_command

VikramGoyal23 added 4 commits October 28, 2025 18:42

Add tags push

da38556

Add download.py

4073270

Add verify.py

96c3409

Add unit tests and clean verify.py

7fb80a2

keerthigkaarthik mentioned this pull request Nov 1, 2025

[Exercise Discussion] T6L2/branch-forward #66

Open

1 task

damithc linked an issue Nov 2, 2025 that may be closed by this pull request

[Exercise Discussion] T4L2/tags-push #63

Open

1 task

woojiahao added the exercise review Review a proposed exercise label Nov 10, 2025

woojiahao self-assigned this Nov 20, 2025

woojiahao requested changes Nov 20, 2025

View reviewed changes

woojiahao added the discussing label Nov 20, 2025

Implement feedback

ef903c4

- Added instructions to README - Removed hacks and cleaned up download.py - Removed GH API call - Cleaned up test_verify.py - Added new test

woojiahao requested changes Nov 27, 2025

View reviewed changes

Fix formatting

9f15849

jovnc reviewed Dec 4, 2025

View reviewed changes

Fix whitespace

b0cb09e

	run_command(["git", "tag", "-a", "v2.0", "HEAD~1", "-m", f"\"{"First stable roster"}\""], verbose)
	run_command(["git", "tag", "-a", "v2.0", "HEAD~1", "-m", "First stable roster"], verbose)

Implement exercise T4L2/tags-push #95

Are you sure you want to change the base?

Implement exercise T4L2/tags-push #95

Uh oh!

Conversation

VikramGoyal23 commented Oct 28, 2025

Exercise Review

Exercise Discussion

Checklist

Uh oh!

VikramGoyal23 commented Oct 28, 2025

Uh oh!

VikramGoyal23 commented Oct 29, 2025

Uh oh!

woojiahao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VikramGoyal23 commented Nov 21, 2025

Uh oh!

VikramGoyal23 commented Nov 24, 2025

Uh oh!

woojiahao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VikramGoyal23 commented Dec 2, 2025

Uh oh!

jovnc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants