Implement exercise T6L2/ff-undo #101
Conversation
woojiahao
left a comment
woojiahao
left a comment
There was a problem hiding this comment.
Left some comments, the structure for autograding makes sense, but could be improved
ff_undo/verify.py
Outdated
| has_birdperson = any("Add Birdperson" in msg for msg in commit_messages_in_others) | ||
| has_cyborg = any("Add Cyborg to birdperson.txt" in msg for msg in commit_messages_in_others) | ||
| has_tammy = any("Add Tammy" in msg for msg in commit_messages_in_others) | ||
| if len(commit_messages_in_others) != 6 or not (has_birdperson and has_cyborg and has_tammy): |
There was a problem hiding this comment.
A little OOTL, but why is this check for 6, not 3?
There was a problem hiding this comment.
When retrieving commits from the others branch, it also includes the commits from main that existed before the branch was created. Therefore, the total number of commits is 5 instead of 3 (since I deleted the first tag, it now counts as 5).
|
LGTM! I'll wait for @woojiahao's input on this. |
jovnc
left a comment
jovnc
left a comment
There was a problem hiding this comment.
Thank you for the contribution! Some changes are needed before we can merge this PR, as there seems to be some issues with the verification logic that affect the correctness of the program
There was a problem hiding this comment.
This file should be removed
|
|
||
| # Check that commits in others are only the initial 3 commits | ||
| if len(commit_messages_in_others) != 5 or not all( | ||
| msg in commit_messages_in_others for msg in [ADD_BIRDPERSON, ADD_CYBORG, ADD_TAMMY] |
There was a problem hiding this comment.
| msg in commit_messages_in_others for msg in [ADD_BIRDPERSON, ADD_CYBORG, ADD_TAMMY] | |
| msg in commit_messages_in_others for msg in [ADD_BIRDPERSON, ADD_CYBORG, ADD_TAMMY, ADD_RICK, ADD_MORTY] |
Would be good to verify all 5 commits exist in the other branch
There was a problem hiding this comment.
It should also be 6, seems like when we set up the exercise using test-download.sh there is 6 commits including the initial commit.
There was a problem hiding this comment.
This likely will require updating the repo-smith tests as well to include the initial commit
|
|
||
| ## Task | ||
|
|
||
| You have a repository with two branches: |
There was a problem hiding this comment.
Should we use gitGraph mermaid diagrams here instead? This can be copied from the issue description.
gitGraph BT:
commit id: "Add Rick"
commit id: "[main] Add Morty"
branch others
commit id: "Add Birdperson"
commit id: "Add Cyborg to birdperson.txt"
commit id: "[HEAD → other] Add Tammy"
There was a problem hiding this comment.
README.md can simply point to https://git-mastery.github.io/exercises-directory/index.html
There was a problem hiding this comment.
Currently the website isn't updated with the new exercises, hence I forsee that it might lead to some inconvenience during beta testing stage when we need to refer to instructions for the exercise. Perhaps, we can update all the README.md to point to the website once the exercises are ready after testing.
Here's an example of README.md that we can have next time, given that the URL fragment identifier follows exercise-<EXERCISE_NAME>
Refer to [here](https://git-mastery.github.io/exercises-directory/index.html#exercise-ff-unfo) for more informationThere was a problem hiding this comment.
Yup. We should do this for all exercises. In future, I'll add the exercise description to the website when I post the issue.
5 participants
Exercise Review
Exercise Discussion
#89
Checklist
Git-Masteryorganization, have you created a request for it?repo-smithto validate the exercise grading scheme?test-download.sh?git-autograder?app?