[Droid-An]

Learners, PR Template

Self checklist

• I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
• My changes meet the requirements of the task
• I have tested my changes
• My changes follow the style guide

Task code

Replace this line with the Task code (e.g. CYF-0000).

Changelist

Briefly explain your PR.

Questions

Ask any questions you have for your reviewer. You must remove this section if you have no questions.

[cyf-ai-code-reviewer]

In the new test_sdc_tasks job, the same or very similar long boolean expressions are repeated in multiple if: conditions (for example, the list of steps.*.outputs.attempted checks in make output comment and fail if not complete, and the list of steps.*.outputs.complete checks in add appropriate labels).

This isn’t a functional bug, but it does mean that whenever you add or remove a task (e.g. another directory to test), you’d need to remember to update several separate if: expressions in exactly the same way. That’s often how inconsistencies creep in over time.

Would it be possible to centralise the notion of "any task attempted" and "any task complete" using a single step whose output you reuse, or by grouping these conditions in one place (for example via a small helper step that sets a combined output flag)? That way, adding a new task would only require updating one location, and the job-level conditions would stay in sync automatically.

To "like" or "dislike" this comment, please follow this link

[cyf-ai-code-reviewer]

Several branches of this script follow a very similar pattern: change into a task directory, loop over exercises, run each exercise to test.tmp, cmp against an expect file, maintain pass/total counters, and finally print a summary and set GITHUB_OUTPUT flags. Variants of this appear in the individual-shell-tools, shell-pipelines, and jq sections, and you also repeat rm test.tmp logic in multiple places.

When you have this kind of structural repetition, it becomes harder to keep behaviour consistent if you later tweak the testing logic (for example, if you change how you report failures or how you handle temporary files). You’d then need to apply the same change in several places and it’s easy for one branch to drift.

Could you imagine extracting the common “run command list, compare to expect files, track pass/total, update outputs” behaviour into a reusable function (or at least a single shared block) that takes the base directories as parameters? That would let each case just describe what to test, and the shared code would handle the mechanics consistently for all three task types.

To "like" or "dislike" this comment, please follow this link

[cyf-ai-code-reviewer]

These top-of-file comments are all describing exactly what the script already does: it’s a bash script that will be run on PRs, shouldn’t be modified, and can be run with an argument. Because the behaviour is quite obvious from the shebang, filename and the ./test-sdc.sh task-directory-name usage, the comments don’t really add extra understanding.

When you add comments like this, it can be worth asking yourself: would a future reader be confused without this, or could they infer it from the code and context? If the answer is “they can already see it”, the comment might just add noise and become something that can go out of date later (for example, if someone ever changes how it’s invoked but forgets to update the comment).

How might you decide which of these high-level remarks are genuinely useful context (e.g. a one-line note that this file is run in CI) and which ones could be omitted because the code or filename already states the same thing?

To "like" or "dislike" this comment, please follow this link

[cyf-ai-code-reviewer]

The CYF AI review has left comments. It will only review a PR one time. When you have addressed these comments, please request review again and a volunteer will take a look.