Repository navigation
Mirror 680 - #16
Mirror 680#16Droid-An wants to merge 54 commits into
Conversation
Fix expected output: change 'with' to 'wIth' in sed exercise instructions
* Expected output now in expected folder * Github action which runs after validation, when Needs Review * Script that can be run to test locally before push * Split number sys qs into automatable/manualy checkable --------- Co-authored-by: l <l>
* Add a shebang because `-v` is a bash-ism not present in whatever my default shell is on macOS. * Remove `--quiet` from `cmp` - macOS's `cmp` doesn't support it.
* Trigger on added files - Cowsay involves writing brand new files * Hide previous comments * Fix completed -> complete * Clean up output a little
So a trainee has to convert the hex into a number before considering how this might look as colours. Ideally to improve lead into the idea of that % of bytes used can create the colour mix. Same could be said for the grey question needing more suggestion of how to calculate a colour mix (somehow hinting or leading to the idea of proportion of bytes used). Suggestions welcome for whether this fixes a misunderstanding or not, or if it can be improved.
Some trainees are doing a bit less than I was expecting when originally setting this. Not to be merged util the current cohort have completed Module-Tools.
…d through functions as arguments - also fixed an if boolean
Droid-An
added
NotCoursework
Needs Review
cyf-ai-code-reviewer
Bot
added
Reviewed
and removed
Needs Review
![cyf-ai-code-reviewer[bot]](https://avatars.githubusercontent.com/u/22743767?s=60&v=4){kind=small}
| - name: checkout base branch | ||
| uses: actions/checkout@v5 |
There was a problem hiding this comment.
The step name checkout base branch suggests that this action is checking out the base branch of the pull request, but the actions/checkout@v5 step is used without specifying a ref. On a pull_request event, that means it actually checks out the merge commit (effectively the PR’s code), not the base branch. For someone reading this workflow, that name could be misleading when trying to understand what is being tested.
You might find it useful to ask yourself: if another teammate read only the step names to understand the flow, would they come away with the correct picture of what is happening? Here, the behavior and the name don’t quite match, which can cause confusion when debugging CI runs or when you later want to actually check out the base branch for comparison.
To "like" or "dislike" this comment, please follow this link
| def double(number): | ||
| return number * 3 | ||
|
|
There was a problem hiding this comment.
The function is called double, but it multiplies the input by 3 instead of 2. The name and the implementation intentionally don’t match, but from a naming point of view this is exactly the kind of mismatch that can trip people up in real code: a reader would reasonably assume double(10) returns 20, not 30.
When you spot this kind of thing, it can be good to think: should I change the name so it matches the behavior (e.g. a name that suggests tripling), or should I change the behavior so it matches the name? In production code, keeping those two in sync helps others (and your future self) trust function names without constantly re-reading the implementation.
To "like" or "dislike" this comment, please follow this link
|
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. |
Learners, PR Template
Self checklist
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.