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
Open
SAN-MUYUN wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Implement exercise T4L1/view-commits #102

SAN-MUYUN wants to merge 3 commits into from

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
Open
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!

view_commits/download.py Outdated
from exercise_utils.gitmastery import create_start_tag

def setup(verbose: bool = False):
create_start_tag(verbose)
Copy link
Member

@woojiahao woojiahao Nov 21, 2025

Choose a reason for hiding this comment

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

I don't think we need this. We can just leave this function as ...

view_commits/verify.py Outdated
Comment on lines 17 to 24
.add_validation(QUESTION_TWO, NotEmptyRule())
.add_validation(QUESTION_TWO, HasExactValueRule("Bruce", is_case_sensitive=True))
.add_validation(QUESTION_THREE, NotEmptyRule())
.add_validation(QUESTION_THREE, HasExactListRule(["Betsy", "Beth", "Daisy"], is_case_sensitive=True))
.add_validation(QUESTION_THREE, ContainsListRule(["Betsy", "Beth", "Daisy"], subset=True, is_case_sensitive=True))
.add_validation(QUESTION_FOUR, NotEmptyRule())
.add_validation(QUESTION_FOUR, HasExactListRule(["Charlie"], is_case_sensitive=True))
.add_validation(QUESTION_FOUR, ContainsListRule(["Charlie"], subset=True, is_case_sensitive=True))
Copy link
Member

@woojiahao woojiahao Nov 21, 2025

Choose a reason for hiding this comment

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

The add_validation function takes in a variadic list of arguments, so you can pass all of the rules for a given question within the same call to add_validation!

view_commits/tests/test_verify.py
Copy link
Member

@woojiahao woojiahao Nov 21, 2025

Choose a reason for hiding this comment

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

Thank you for writing such comprehensive test specs for the answers.txt! But it's completely okay to keep the spec as the base file! When you set mock_answers, it immediately loads in the answers.txt with the answers you give it! So it doesn't read off of the unit tested file.

Copy link
Contributor Author

@SAN-MUYUN SAN-MUYUN Dec 7, 2025

Choose a reason for hiding this comment

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

Thanks for this feedback! I tried to simplify it and keep the spec as base file. But I run into some OS permission problem again. My repo-smith is updated to v1.0.0. It works perfect if I keep it as it was.
image

@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
Contributor

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
Contributor

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 .

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

Reviewers

@woojiahao woojiahao woojiahao requested changes

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

3 participants

@SAN-MUYUN @jovnc @woojiahao