[glossary-branch-rename] Improve verify logic and update tests#263
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": "fix/glossary-branch-rename"
}
}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. |
SAN-MUYUN
left a comment
SAN-MUYUN
left a comment
There was a problem hiding this comment.
Thanks for fixing this bug! Apart from small adjustment to the comment and RESET_MESSAGE, other changes LGTM!
glossary_branch_rename/verify.py
Outdated
|
|
||
| This is necessary since the renames could be performed in parts: | ||
|
|
||
| login -> feat/login -> feature/login |
There was a problem hiding this comment.
Since this method is exercise specific, let's make the comment more relevant to our exercise
glossary_branch_rename/verify.py
Outdated
| STU_REMOTE_PRESENT = f"Remote branch {STU_BRANCH} still exists." | ||
| RENAMED_REMOTE_MISSING = f"Remote branch {RENAMED_BRANCH} is missing." | ||
| NO_RENAME_EVIDENCE = f"Local branch '{STU_BRANCH}' was not renamed to '{RENAMED_BRANCH}'!" | ||
| RESET_MESSAGE = 'Reset the repository using "gitmastery progress reset" and start again' |
There was a problem hiding this comment.
I like the RESET_MESSAGE as it is possible that the users do somethings wrong and unable to continue.
I think it would be better if we change the message to If needed, ... and start again.
When users failed to rename the branches, it is not necessary that they always end up in a state where there is no return and have to go for reset. Don't forget the fullstop at the end :)
| if original == old_branch: | ||
| old_branch = new | ||
|
|
||
| return old_branch == new_branch |
|
@desmondwong1215 @SAN-MUYUN Feel free to merge when ready |
3 participants
Fixes #262