Implement exercise T6L2/merge-squash #148
Conversation
There seems to be some issue with the squash option for the merge step in repo-smith
|
This pull request is not yet complete, the testing needs to be cleaned up. |
jovnc
left a comment
jovnc
left a comment
There was a problem hiding this comment.
LGTM, tests looks comprehensive enough, some minor changes needed on verification process before merging.
merge_squash/verify.py
Outdated
|
|
||
| commit_messages_in_main = [c.commit.message.strip() for c in main_branch.commits] | ||
|
|
||
| merge_commits = [c for c in main_branch.commits if len(c.parents) > 1] |
There was a problem hiding this comment.
I think the logic here is that we want to ensure a linear history by ensuring that there are no commits with multiple parents (that can indicate a merge commit).
But I think that it is also possible to pass this verification using git -i rebase.
However, I'm not sure if there's a better way to verify squash merge, so this LGTM for now
There was a problem hiding this comment.
Good point, I was wary of using commit messages for verification purposes but I think it's best to check if the latest commit message has the word "Squash" to ensure a proper squash merge.
|
I moved around some of the verification logic as well, since to me, it made more sense to check if the proper type of merge was used after checking if the changes were even present. |
jovnc
left a comment
jovnc
left a comment
There was a problem hiding this comment.
I think that we could be safer with more tests, and the verification logic can be improved
jovnc
left a comment
jovnc
left a comment
There was a problem hiding this comment.
Just a last nit on verify.py, but other than that everything else LGTM
| def verify(exercise: GitAutograderExercise) -> GitAutograderOutput: | ||
| main_branch = exercise.repo.branches.branch("main") | ||
|
|
||
| with exercise.repo.files.file_or_none("mike.txt") as mike_file: |
There was a problem hiding this comment.
We might need to handle the case where we perform a squash merge, but the files are not committed, as the current test checks for existence of file, but doesn't check that we have committed the squash merge.
eg. git merge --squash supporting but we don't commit the changes would still pass the tests
Perhaps we could check that there are no uncommitted files, but we cannot replicate this is repo-smith currently.
if exercise.repo.repo.is_dirty():
raise exercise.wrong_answer([SQUASH_NOT_COMMITTED])
|
I've added a check for any uncommitted files. Unfortunately, the |
jovnc
left a comment
jovnc
left a comment
There was a problem hiding this comment.
LGTM, thanks for addressing the comments!
2 participants
Exercise Review
Exercise Discussion
#68
Checklist
Git-Masteryorganization, have you created a request for it?repo-smithto validate the exercise grading scheme?test-download.sh?git-autograder?app?