Skip to content

Implement exercise T4L1/view-commits #102

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.

Sign up for GitHub

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

Jump to bottom
Merged
jovnc merged 6 commits into from
Dec 16, 2025
Merged

Implement exercise T4L1/view-commits #102

jovnc merged 6 commits into from
Dec 16, 2025

Conversation

@SAN-MUYUN
Copy link
Contributor

@SAN-MUYUN SAN-MUYUN commented Nov 1, 2025

Exercise Review

Exercise Discussion

#60

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?

@damithc damithc linked an issue Nov 2, 2025 that may be closed by this pull request
[Exercise Discussion] T4L1/view-commits #60
Closed
1 task
@woojiahao woojiahao added the exercise review Review a proposed exercise label Nov 10, 2025
woojiahao
woojiahao requested changes Nov 21, 2025
View reviewed changes
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.

Thank you @SAN-MUYUN for helping to work on this exercise! I've added some comments!

@SAN-MUYUN
Copy link
Contributor Author

SAN-MUYUN commented Nov 26, 2025

Thanks so much for the feedback! I will make the necessary changes hopefully by next week.

@jovnc
Copy link
Collaborator

jovnc commented Dec 6, 2025

Hi @SAN-MUYUN , 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!

@SAN-MUYUN
Copy link
Contributor Author

SAN-MUYUN commented Dec 7, 2025 •
edited
Loading

@woojiahao Apologies for the long wait! Apart from the test specs, I have updated the PR accordingly.

I just realised that I cannot pass the verify-tests cases for grocery-shopping as well, it is prompting the same error as #74 despite my repo-smith is updated to v1.0.0.

@jovnc
Copy link
Collaborator

jovnc commented Dec 7, 2025

@SAN-MUYUN I have verified that the tests passes on my MacOS machine.

@SAN-MUYUN
Copy link
Contributor Author

SAN-MUYUN commented Dec 7, 2025

@jovnc Thanks for the update! I am using Windows 11 OS. Similar issues have happened before, likely because of how repo-smith handles deletion of temporary folders as mentioned in issue #74 .

@jovnc
Copy link
Collaborator

jovnc commented Dec 12, 2025

@VikramGoyal23 Would it be possible to help test on a Windows machine as well, and see if the same issue persists? Thanks!

@VikramGoyal23
Copy link
Collaborator

VikramGoyal23 commented Dec 12, 2025

OS: Windows 11

All tests are passing.

image

However, other repos are still throwing permission errors similar to issue #74. Below are branch-bender's tests:

image

jovnc
jovnc approved these changes Dec 12, 2025
View reviewed changes
Copy link
Collaborator

@jovnc jovnc left a comment

Choose a reason for hiding this comment

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

Other than OS-specific issue with running repo-smith tests, the verification logic and testing LGTM. The OS related issue can be solved separately in a different PR, though I cannot pinpoint exactly what is causing the issue.

@VikramGoyal23 @woojiahao Any thoughts on this? Feel free to merge when ready.

@jovnc jovnc requested a review from VikramGoyal23 December 15, 2025 05:27
@jovnc
Copy link
Collaborator

jovnc commented Dec 16, 2025

@SAN-MUYUN I have helped to clean up the test_verify.py to keep only the base files, and also run formatting on the code. Thank you for your contribution @SAN-MUYUN, I think that it is good enough for now, and we can merge this!

@jovnc jovnc merged commit 998c55b into git-mastery:main Dec 16, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@woojiahao woojiahao woojiahao requested changes

@jovnc jovnc jovnc approved these changes

@VikramGoyal23 VikramGoyal23 Awaiting requested review from VikramGoyal23

Assignees

No one assigned

Labels

discussing exercise review Review a proposed exercise

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Exercise Discussion] T4L1/view-commits

4 participants

@SAN-MUYUN @jovnc @VikramGoyal23 @woojiahao