[grocery-shopping] Improve verify logic #232
Conversation
|
Hi @desmondwong1215, 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": "desmondwong1215",
"repository": "exercises",
"branch": "e-grocery-shopping",
}
}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
left a comment
jovnc
left a comment
There was a problem hiding this comment.
Left some preliminary comments, will check test logic later on
| repo_folder = exercise.config.exercise_repo.repo_name | ||
| work_dir = os.path.join(repo_root, repo_folder) | ||
|
|
||
| shopping_list_file_path = os.path.join(work_dir, "shopping-list.txt") |
There was a problem hiding this comment.
There seems to be duplicate check now, we can remove the check for the shopping-list.txt below since we are already checking it here, since this is step 1.
grocery_shopping/verify.py
Outdated
| main_branch = exercise.repo.branches.branch("main") | ||
|
|
||
| # Verify that not all commits are empty | ||
| if not main_branch.has_non_empty_commits(): |
There was a problem hiding this comment.
This check is no longer necessary, we also shouldn't be too restrictive in our checks here. As long as students get to the goal, we should still allow empty commits.
grocery_shopping/verify.py
Outdated
|
|
||
| # Check if they edited the shopping-list.md at least once | ||
| # Check if they edited the shopping-list.txt at least once | ||
| if not main_branch.has_edited_file("shopping-list.txt"): |
There was a problem hiding this comment.
This check doesn't seem necessary as well anymore. The above already checks that we have editted the file, below already checks that we have the file change has been committed.
grocery_shopping/test_verify.py
Outdated
|
|
||
|
|
||
| def test_no_remove(): | ||
| def test_other_file_changes_committed(): |
There was a problem hiding this comment.
This test shouldn't be necessary (refer to comments below). If they made comments on the wrong file, it should fail earlier isn't it, since shopping-list.txt should no longer have the expected content
|
Hi @desmondwong1215, 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": "desmondwong1215",
"repository": "exercises",
"branch": "e-grocery-shopping",
}
}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. |
2 participants
Exercise Review
Exercise Discussion
Fixes #225
Checklist
Git-Masteryorganization, have you created a request for it?repo-smithto validate the exercise grading scheme?git-autograder?app?