Skip to content

Conversation

@LeeZeHao
Copy link

Exercise Review

Exercise Discussion

#86

Checklist

  • If you require a new remote repository on the Git-Mastery organization, have you created a request for it?
  • Have you written unit tests using repo-smith to validate the exercise grading scheme?
  • Have you tested the download script using test-download.sh?
  • Have you verified that this exercise does not already exist or is not currently in review?
  • Did you introduce a new grading mechanism that should belong to git-autograder?
  • Did you introduce a new dependency that should belong to app?

Copy link
Member

@woojiahao woojiahao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some minor nits about coding convention!

from exercise_utils.gitmastery import create_start_tag

__requires_git__ = True
__requires_github__ = True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're not using gh, you don't need this to be set to true

Suggested change
__requires_github__ = True
__requires_github__ = False


def download(verbose: bool):
os.makedirs("samplerepo-finances")
# Clone the samplerepo-finances repo
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Clone the samplerepo-finances repo

# Clone the samplerepo-finances repo
run_command(["git", "clone", "https://github.com/git-mastery/samplerepo-finances.git"], verbose)
os.chdir("samplerepo-finances")
# Change the remote origin to point to samplerepo-finances-2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Change the remote origin to point to samplerepo-finances-2

Comment on lines 17 to 18

pass
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pass

once the function behavior is populated, you are free to remove pass

@LeeZeHao
Copy link
Author

@woojiahao understood, implemented suggested changes in latest commit.

Copy link
Member

@woojiahao woojiahao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left comments

Comment on lines 11 to 13
os.makedirs("samplerepo-finances")
run_command(["git", "clone", "https://github.com/git-mastery/samplerepo-finances.git"], verbose)
os.chdir("samplerepo-finances")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this set of steps are necessary. If you run clone_repo_with_git, it should produce the folder you need.

Suggested change
os.makedirs("samplerepo-finances")
run_command(["git", "clone", "https://github.com/git-mastery/samplerepo-finances.git"], verbose)
os.chdir("samplerepo-finances")
clone_repo_with_git("https://github.com/git-mastery/samplerepo-finances.git", verbose)
os.chdir("samplerepo-finances")

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed, new version uses clone_repo_with_git function.

os.makedirs("samplerepo-finances")
run_command(["git", "clone", "https://github.com/git-mastery/samplerepo-finances.git"], verbose)
os.chdir("samplerepo-finances")
run_command(["git", "remote", "set-url", "origin", "https://github.com/git-mastery/samplerepo-finances-2.git"], verbose)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was not able to recreate the output in https://nus-cs2103-ay2526s1.github.io/website/book/gitAndGithub/pull/ exactly using add_remote and remove remote. Falling back to run_command instead.

@jovnc
Copy link
Contributor

jovnc commented Dec 6, 2025

Hi @LeeZeHao, is this PR currently being worked on?

Would be good to get an update by 9 Dec, else we need to close this PR, so that we can resolve this issue. Thanks!

@LeeZeHao
Copy link
Author

LeeZeHao commented Dec 6, 2025

@jovnc @woojiahao I have made the latest commits.
Apologies for being late on updates, I was busy with final exams.

"https://github.com/git-mastery/samplerepo-finances.git", verbose
)
os.chdir("samplerepo-finances")
run_command(
Copy link
Contributor

@jovnc jovnc Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use this instead from exercise_utils.git

remove_remote("origin", verbose)
add_remote("origin", "https://github.com/git-mastery/samplerepo-finances-2", verbose)

This is equivalent to a set-url. Alternatively, you can create a new utility to do this under exercise-utils but that may be unnecessary.

This is a similar merged PR #116

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Hands-On Discussion] T3L3/hp-fetch-merge (Fetch and merge from a remote)

3 participants