Implement exercise T4L5/sensors-reset #164

jovnc · 2025-12-15T07:42:38Z

Exercise Review

Exercise Discussion

Fixes #145

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?

jovnc · 2025-12-15T07:43:41Z

Repo-smith currently does not support reset step, hence we need to implement this.

In addition, when cloning from remote repository, the default branch seems to be still master branch instead of main branch. We might need to streamline this for remote repositories as well.

damithc · 2025-12-15T07:54:20Z

In addition, when cloning from remote repository, the default branch seems to be still master branch instead of main branch. We might need to streamline this for remote repositories as well.

Isn't the default branch is inherited from the cloned repo? i.e., same as the cloned repo

jovnc · 2025-12-15T07:59:36Z

@damithc Yes, the default branch is inherited from the cloned repo. In this case, when we clone https://github.com/git-mastery/gm-sensors, the default branch would be master. However, most of the tooling in repo-smith and this folder itself, eg. init functions seems to assume main as default.

eg. repo-smith tests defaults to main by default
eg. init in exercise_utils uses main by default

This inconsistency can lead to documentation inconsistencies, as well as some issues in verification process, eg. as shown below. We expect master branch since when we clone the default branch is master. However, for repo-smith tests, it assumes default is main, hence causing errors in testing.

master_branch = exercise.repo.branches.branch_or_none("master")

Though a possible solution would be

branch = exercise.repo.branches.branch_or_none("master") or exercise.repo.branches.branch_or_none("main")

damithc · 2025-12-15T08:19:31Z

@damithc Yes, the default branch is inherited from the cloned repo. In this case, when we clone https://github.com/git-mastery/gm-sensors, the default branch would be master. However, most of the tooling in repo-smith and this folder itself, eg. init functions seems to assume main as default.

eg. repo-smith tests defaults to main by default eg. init in exercise_utils uses main by default

This inconsistency can lead to documentation inconsistencies, as well as some issues in verification process, eg. as shown below
master_branch = exercise.repo.branches.branch_or_none("master")
Though a possible solution would be
branch = exercise.repo.branches.branch_or_none("master") or exercise.repo.branches.branch_or_none("main")

hmm... we probably have to migrate the entire git-mastery to use one (main or master) as the default branch. I don't mind going for the more modern choice main but it is not easy to update screenshots and videos to match, and it is incompatible with other CS2103 materials e.g., iP, tP, lecture videos. The other option is to switch everything in Git-Mastery to master (at least the parts facing students). How much work is that?

jovnc · 2025-12-15T08:42:56Z

@damithc I remember it being discussed somewhere (though I cannot trace the exact issue), that it was decided that main was the default branch for git-mastery.

But, given the constraints, I think it could be possible to switch everything to master.

Regarding effort needed, we would need to:

update all download.py for exercises if it uses main
update all hands on that uses main
update git-mastery website (as some hands on uses main, some exercises uses master, so effort is needed to update the website if we want to standardise to one)
update repo-smith to default to master

Alternatively, we can keep what we have currently at the cost of some (technical) debt. This means for example, in this exercise, we cannot assume main or master as default branch and must check both. This is the fastest solution, but would require some clarification with students early in the course, that default branch can either be main or master

@VikramGoyal23 @woojiahao What are your thoughts on this?

VikramGoyal23 · 2025-12-15T10:11:40Z

@damithc I remember it being discussed somewhere (though I cannot trace the exact issue), that it was decided that main was the default branch for git-mastery.

But, given the constraints, I think it could be possible to switch everything to master.

Regarding effort needed, we would need to:
* update all `download.py` for exercises if it uses `main`

* update all hands on that uses `main`

* update git-mastery website (as some hands on uses `main`, some exercises uses `master`, so effort is needed to update the website if we want to standardise to one)

* update repo-smith to default to `master`
Alternatively, we can keep what we have currently at the cost of some (technical) debt. This means for example, in this exercise, we cannot assume main or master as default branch and must check both. This is the fastest solution, but would require some clarification with students early in the course, that default branch can either be main or master

@VikramGoyal23 @woojiahao What are your thoughts on this?

Yes, I discussed this issue in git-mastery/git-mastery#49. During an in-person discussion, Damith mentioned the value of using both interchangeably to get students familiarized with different default branches. Certainly, we can try to fully commit to one branch or the other, but either would require much effort- main is the assumed branch for the majority of the exercise infrastructure, while master is more in-line with the course contents. I think the middle-of-the-road option would be the least painful in the long run, especially since we would have to clarify the possibility of different default branches to the students anyhow to prepare them for the real world.

VikramGoyal23 · 2025-12-23T15:54:08Z

@jovnc Can you uncomment all the unit tests and run them? I can give a proper review once that's done.

jovnc · 2025-12-23T16:52:42Z

@VikramGoyal23 Had to make some major changes to the tests and verify logic, as I realised some limitations in the existing verify logic (as I can only verify the end state, and not the in between state for each task).

VikramGoyal23

LGTM, just need to update the branch logic and it can be merged.

sensors_reset/verify.py

VikramGoyal23 · 2025-12-24T10:03:48Z

sensors_reset/verify.py

+    comments: str = []
+    if len(unstaged_files) != 4 or not all(
+        file in unstaged_files
+        for file in ["east.csv", "north.csv", "south.csv", "west.csv"]
+    ):
+        comments.append(WRONG_FILES_IN_WORKING_DIRECTORY)
+
+    if len(staged_files) != 3 or not all(
+        file in staged_files for file in ["east.csv", "north.csv", "south.csv"]
+    ):
+        comments.append(WRONG_FILES_IN_STAGING_AREA)


While I initially thought that this logic could be easily circumvented by choosing a set of incorrect commits to reset to, such a set of commits (other than the one mentioned in the exercise instructions) does not exist, making this logic sound in all cases.

VikramGoyal23

Forgive me, I forgot to write down one more small nit I had.

VikramGoyal23 · 2025-12-24T10:07:55Z

sensors_reset/verify.py

+    if "Record data for Jan 12" in commit_messages:
+        raise exercise.wrong_answer([CONTAINS_TASK_THREE_COMMIT])
+


Can we explicitly check that the head is at January 11th after this for extra safety?

jovnc · 2025-12-24T12:57:33Z

@VikramGoyal23 I have made the changes, thanks!

VikramGoyal23 · 2025-12-24T13:20:33Z

sensors_reset/verify.py

+
+
+def verify(exercise: GitAutograderExercise) -> GitAutograderOutput:
+    branch = exercise.repo.branches.branch_or_none("main")


Technically we don't need branch_or_none but it's fine to keep here.

VikramGoyal23

LGTM!

[sensors-reset] Implement verify.py and download.py

29996c5

jovnc self-assigned this Dec 15, 2025

jovnc changed the title ~~[sensors-reset] Implement exercise T4L5/sensors-reset~~ Implement exercise T4L5/sensors-reset Dec 15, 2025

jovnc added priority.High exercise review Review a proposed exercise discussing labels Dec 16, 2025

jovnc mentioned this pull request Dec 18, 2025

Add reset step git-mastery/repo-smith#8

Merged

[sensors-reset] Update repo-smith tests

c1d19a3

jovnc marked this pull request as ready for review December 18, 2025 16:09

[sensors-reset] Clean up code, change ref to revision

8b5f722

VikramGoyal23 self-requested a review December 23, 2025 12:18

jovnc added 2 commits December 24, 2025 00:51

[sensors-reset] Refactor verify logic

f804119

[sensors-reset] Update repo-smith tests with new verify logic

79eceb6

VikramGoyal23 requested changes Dec 24, 2025

View reviewed changes

[sensors-reset] Add verification for head commit

2a2d457

VikramGoyal23 reviewed Dec 24, 2025

View reviewed changes

VikramGoyal23 approved these changes Dec 24, 2025

View reviewed changes

VikramGoyal23 merged commit 086fffb into git-mastery:main Dec 24, 2025
3 checks passed

		if "Record data for Jan 12" in commit_messages:
		raise exercise.wrong_answer([CONTAINS_TASK_THREE_COMMIT])



		def verify(exercise: GitAutograderExercise) -> GitAutograderOutput:
		branch = exercise.repo.branches.branch_or_none("main")

Implement exercise T4L5/sensors-reset #164

Implement exercise T4L5/sensors-reset #164

Uh oh!

Conversation

jovnc commented Dec 15, 2025

Exercise Review

Exercise Discussion

Checklist

Uh oh!

jovnc commented Dec 15, 2025

Uh oh!

damithc commented Dec 15, 2025

Uh oh!

jovnc commented Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

damithc commented Dec 15, 2025

Uh oh!

jovnc commented Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VikramGoyal23 commented Dec 15, 2025

Uh oh!

VikramGoyal23 commented Dec 23, 2025

Uh oh!

jovnc commented Dec 23, 2025

Uh oh!

VikramGoyal23 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

VikramGoyal23 Dec 24, 2025

Choose a reason for hiding this comment

Uh oh!

VikramGoyal23 left a comment

Choose a reason for hiding this comment

Uh oh!

VikramGoyal23 Dec 24, 2025

Choose a reason for hiding this comment

Uh oh!

jovnc commented Dec 24, 2025

Uh oh!

VikramGoyal23 Dec 24, 2025

Choose a reason for hiding this comment

Uh oh!

VikramGoyal23 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jovnc commented Dec 15, 2025 •

edited

Loading

jovnc commented Dec 15, 2025 •

edited

Loading