Implement T8L2/glossary-branch-pull#242
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-glossary-branch-pull"
}
}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. |
|
@desmondwong1215 Can you indicate by mentioning us when this PR is ready for our review, as it seems you're still working on it? Another method you could use is to mark this PR as a draft until it's ready to be reviewed. |
|
Hi @VikramGoyal23, this PR is ready for review. Thanks |
VikramGoyal23
left a comment
There was a problem hiding this comment.
Mostly LGTM. Just a nit and some minor changes to logic for now, while we wait on the update to repo-smith to support unit tests.
glossary_branch_pull/download.py
Outdated
|
|
||
| checkout("DEF", False, verbose) | ||
| run_command(["git", "reset", "--hard", "HEAD~1"], verbose) | ||
| create_or_update_file("e.txt", "documentation: Evidence that someone once cared.\n") |
There was a problem hiding this comment.
It is more idiomatic to use a multiline string here.
| create_or_update_file("e.txt", "documentation: Evidence that someone once cared.\n") | |
| create_or_update_file( | |
| "e.txt", | |
| """ | |
| documentation: Evidence that someone once cared. | |
| """ | |
| ) |
|
@VikramGoyal23 Let's get this merged soon |
|
@VikramGoyal23 Let's get this merged soon so tour 8 can be released to students |
…e-glossary-branch-pull
VikramGoyal23
left a comment
There was a problem hiding this comment.
Some changes in verification logic are required, and I have a few nits, but otherwise it mostly LGTM.
glossary_branch_pull/download.py
Outdated
|
|
||
| checkout("DEF", False, verbose) | ||
| run_command(["git", "reset", "--hard", "HEAD~1"], verbose) | ||
| create_or_update_file("d.txt", "documentation: Evidence that someone once cared.\n") |
There was a problem hiding this comment.
It is more idiomatic to use a multiline string here.
| create_or_update_file("d.txt", "documentation: Evidence that someone once cared.\n") | |
| create_or_update_file( | |
| "d.txt", | |
| """ | |
| documentation: Evidence that someone once cared. | |
| """ | |
| ) |
glossary_branch_pull/verify.py
Outdated
| try: | ||
| exercise.repo.repo.refs["origin/STU"] | ||
| except (IndexError, KeyError): | ||
| comments.append(BRANCH_NOT_TRACKING.format(branch="STU")) | ||
| pass |
There was a problem hiding this comment.
This doesn't seem to work for checking if the branch is actually tracking, creating a new branch using the -b option also works. The reason for this is that it checks if the local branch and reference to the upstream (origin) branch exist, but not if the local tracks the upstream. Consider changing it back to the old version:
| try: | |
| exercise.repo.repo.refs["origin/STU"] | |
| except (IndexError, KeyError): | |
| comments.append(BRANCH_NOT_TRACKING.format(branch="STU")) | |
| pass | |
| stu_branch = repo.branches.branch("STU").branch | |
| tracking = stu_branch.tracking_branch() | |
| if not tracking or tracking.name != 'origin/STU': | |
| comments.append(BRANCH_NOT_TRACKING.format(branch="STU")) |
glossary_branch_pull/verify.py
Outdated
| try: | ||
| exercise.repo.repo.refs["origin/VWX"] | ||
| except (IndexError, KeyError): | ||
| comments.append(BRANCH_NOT_TRACKING.format(branch="VWX")) | ||
| pass |
There was a problem hiding this comment.
See comment on STU branch checking.
glossary_branch_pull/verify.py
Outdated
| except (IndexError, KeyError): | ||
| comments.append(BRANCH_NOT_TRACKING.format(branch="ABC")) | ||
| pass |
There was a problem hiding this comment.
See comment on STU branch checking.
glossary_branch_pull/verify.py
Outdated
| except (IndexError, KeyError): | ||
| comments.append(BRANCH_NOT_TRACKING.format(branch="DEF")) |
There was a problem hiding this comment.
See comment on STU branch checking.
Exercise Review
Exercise Discussion
Fixes #237
Checklist
Git-Masteryorganization, have you created a request for it?repo-smithto validate the exercise grading scheme?git-autograder?app?