[ignoring-somethings] Improve verify logic#234
Conversation
|
Hi @SAN-MUYUN, thank you for your contribution! 🎉 This PR comes from your fork Before you request for a review, please ensure that you have tested your changes locally! Important The previously recommended way of using Please read the following instructions for the latest instructions. PrerequisitesEnsure that you have the Testing stepsIf you already have a local Git-Mastery root to test, you can skip the following step. Create a Git-Mastery root locally: gitmastery setupNavigate into the Git-Mastery root (defaults to cd gitmastery-exercises/Edit the {
# other fields...
"exercises_source": {
"username": "SAN-MUYUN",
"repository": "git-mastery-exercises",
"branch": "enhancement/ignoring-somethings",
}
}Then, you can use the gitmastery download <your new change>
gitmastery verifyChecklist
Important To any reviewers of this pull request, please use the same instructions above to test the changes. |
jovnc
changed the title
jovnc
changed the title
VikramGoyal23
left a comment
VikramGoyal23
left a comment
There was a problem hiding this comment.
Mostly LGTM, just some minor nits! Also since, I couldn't post it directly in the code, we should consider changing MISSING_COMMITS to You have not committed the relevant changes yet! since the user could have made a commit and changed the .gitignore again, causing them to receive the message despite making the some commits. This allows us to provide helpful feedback for two scenarios at once (Making no changes or commits OR having a dirty tree, i.e., uncommitted changes to .gitignore, which both lead to MISSING_COMMITS being added to feedback).
|
|
||
|
|
There was a problem hiding this comment.
The convention for formatting is to leave 2 lines of whitespace before function definitions.
| # Verify that user has commited the ignore, | ||
| # by comparing the local file and the committed file taken from the repo | ||
| with main_branch.latest_commit.file(".gitignore") as commited_gitignore_file: | ||
| if (commited_gitignore_file is None |
There was a problem hiding this comment.
The committed .gitignore is unlikely to be None unless the student deletes the file and then commits that deletion, before then remaking the file. However, since this is completely possible and we don't have any other error handling for this scenario, this line is a good safety barrier.
| if gitignore_file is None: | ||
| raise exercise.wrong_answer([MISSING_GITIGNORE]) | ||
| gitignore_file_contents = gitignore_file | ||
| no_user_commit = len(main_branch.user_commits) == 0 |
There was a problem hiding this comment.
Could you move the definition of no_user_commit to after line 45? Just to decrease its active scope.
|
@VikramGoyal23 Thanks for the feedback! Updated accordingly |
|
@desmondwong1215 @SAN-MUYUN Also do include closing words like "fix #234" next time when opening PR so that it will link to the issue automatically |
|
LGTM, tested it and it works! |
okk noted |
4 participants
Exercise Review
Exercise Discussion
#226
Checklist
Git-Masteryorganization, have you created a request for it?repo-smithto validate the exercise grading scheme?git-autograder?app?