-
Couldn't load subscription status.
- Fork 14
Implement hands on hp-populate-remote #78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Implement hands on hp-populate-remote #78
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't an easy hands-on to implement because we have remote integration. Left a couple of comments on structuring the hands-on!
Please note that this requires setting up delete access for the GitHub CLI via
gh auth refresh -h github.com -s delete_repo.
@damithc are we able to include this in our setup guide if it's not yet included?
I also realised that the GitHub CLI needs to be configured beforehand for this part of the hands-on to work.
This is already supported when you do __require_github__ = True: https://git-mastery.github.io/developers/docs/architecture/hands-on-structure/#file-structure
hands_on/populate_remote.py
Outdated
| _create_and_commit_fruits_file(verbose) | ||
| _update_fruits_file(verbose) | ||
| _add_additional_files(verbose) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am inclined to oppose to overly abstracting these functions. They do not really need to belong to individual functions at the moment, and the term add_additional_files is not particularly informative for our use case.
We can consider flattening out these functions first, and if we notice a use case for having modularized functions, we can do so in a follow-up PR! What do you think? @oadultradeepfield
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the structure I'm thinking is:
download: setup_local_repository -> create_things_repository -> link_things_repository
And then
setup_local_repository: all behavior in the same function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree! This hands-on .py file does feel a bit too abstracted for its own good. Keeping the structure you suggested makes the most sense, as it meets a nice balance and keeps the abstraction layer meaningful.
We can consider flattening out these functions first, and if we notice a use case for having modularized functions, we can do so in a follow-up PR! What do you think? @oadultradeepfield
I also took a look at some other hands-on issues, and it seems this setup pattern is quite common across them. This one’s pretty similar to #55 as well. So I still believe that the abstractions are useful, just maybe something to refine in a future PR, once modularisation brings clearer benefits (and less head-scratching 😅).
hands_on/populate_remote.py
Outdated
|
|
||
|
|
||
| def _link_repositories(verbose: bool): | ||
| full_repo_name = _get_full_repo_name(verbose) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can likely move this inline with this function. The function to get the full repo name is not used anywhere else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@woojiahao, it is actually used here as well. Is it okay to keep as is?
|
Thanks for the super-detailed comments! I’ll make the corresponding changes later today.
Sorry for the confusion, what I meant was that students need to have the GitHub CLI installed for the |
Given having to delete a remote repo is rare, instead of deleting the repo automatically, a safer alternative is to abort with a message asking the student to delete the repo and run the |
I agree with this approach! If I understand correctly, this would mean the Python script in this repo should raise exceptions that the app’s |
Exercise Discussion
This PR will close #56. Below is a more detailed description of the changes along with implementation notes:
add_originfunction inexercise_utils/git.py. This might be helpful in the future. I noticed that there is already aremove_remoteimplementation, andadd_originis intended to be used in a similar way.gitmastery-thingsis created successfully across multiple attempts, I added a check to delete any existing remote repository with the same name. Please note that this requires setting up delete access for the GitHub CLI viagh auth refresh -h github.com -s delete_repo. I also realised that the GitHub CLI needs to be configured beforehand for this part of the hands-on to work.Note: The download script tested via
test-download.shworks fine. Here is the remote repo I managed to create.Checklist
Git-Masteryorganization, have you created a request for it?repo-smithto validate the exercise grading scheme?test-download.sh?git-autograder?app?