Implement exercise T4L2/tags-update #91
Conversation
|
Opened PR early while waiting to be assigned. Feel free to close if there are duplicates in the future, or if someone else is assigned to this exercise in the future. This would serve as potential reference to others looking for implementations of exercises as well. |
woojiahao
left a comment
woojiahao
left a comment
There was a problem hiding this comment.
Thank you @jovnc for working on this exercise! I think that there are some major comments on the ordering of grading, less on the coding style, more so on the autograding philosophy. I will need to update the public documentation to capture this logic too
| def test_wrong_april_tag(): | ||
| with loader.load("specs/wrong_april_tag.yml", "start") as output: | ||
| assert_output(output, GitAutograderStatus.UNSUCCESSFUL, [WRONG_APRIL_TAG_COMMIT]) |
There was a problem hiding this comment.
I think we would also want to ensure that the same check for the January tag commit should exist too, what do you think?
There was a problem hiding this comment.
That's a good point, would make the tests more comprehensive!
| def tag(tag_name: str, verbose: bool) -> None: | ||
| """Tags the latest commit with the given tag_name.""" | ||
| run_command(["git", "tag", tag_name], verbose) | ||
| def tag(tag_name: str, verbose: bool, commit_ref: str | None = None) -> None: |
There was a problem hiding this comment.
Given that this is repeated multiple times, could we get it as a utility function for future use?
There is an existing tag function under exercise_utils.git, however, it doesn't support tagging a specific commit hash.
We have 2 options:
- extend current utility of the
tagfunction to take in optionalcommit_ref - create a command with a more verbose name
tag_with_committhat serves the same purpose astagbut takes in compulsorycommit_ref
I have chosen the first option, as it keeps backward compatibility, while ensuring simplicity.
There was a problem hiding this comment.
Do I need to open a new PR to add this change for the util, or can I include it in this PR? Is there anywhere I need to document this change?
|
@woojiahao Thanks for the PR review, and the very detailed comments. I have made the changes based on your comments, namely here are the main changes:
|
|
LGTM, let's wait for @woojiahao's input on the change to the |
3 participants
Exercise Review
Exercise Discussion
Fixes #62
Checklist
Git-Masteryorganization, have you created a request for it?repo-smithto validate the exercise grading scheme?test-download.sh?git-autograder?app?