Skip to content

Cape Town | sep-2026-ITP | Matthaus Bause | Sprint 1 | Time Clock - #1648

Open
chefmatthaus-hub wants to merge 4 commits into
CodeYourFuture:mainfrom
chefmatthaus-hub:main
Open

chefmatthaus-hub wants to merge 4 commits into
CodeYourFuture:mainfrom
chefmatthaus-hub:main

Conversation

@chefmatthaus-hub

@chefmatthaus-hub chefmatthaus-hub commented Oct 5, 2026 •

Copy link
Copy Markdown

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

CYF-1197

Fixed formatting issues in time conversion and update test assertions for single-digit morning hours

@chefmatthaus-hub chefmatthaus-hub added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 5, 2026
Comment thread format-clock-edge-cases/timeConverter.test.js
Comment thread format-clock-edge-cases/timeConverter.test.js
@cjyuan cjyuan added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Oct 5, 2026
@chefmatthaus-hub chefmatthaus-hub added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 7, 2026

@cjyuan cjyuan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you include a Changelist section in your PR description?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not all statements are terminated by a semicolon and spacing around { and '}` is not fully consistent.

Suggestion:

  • Look up the benefits of using a code formatter.
  • Install the Prettier extension for VS Code, then:
    • Use VS Code's Format Document feature to format your code.
    • Optionally, enable Format On Save and Format On Paste to keep your code consistently formatted.

Resource: Visual Studio Code - Formatting

Note: The formatter may not work correctly if your code contains syntax errors.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you check whether the expected return values specified in the tests use a consistent format?

When a function's return values are not consistently formatted, it can lead to unintended side effects. For examples:

  1. When the formatted strings are displayed, they may not align nicely:
01:00 am
1:00 pm
12:00 am
01:00 pm
02:00pm
  1. When the formatted strings are compared in a program, they may produce unexpected results:
  • "1:00 pm" < "11:00 pm" evaluates to false
  • "01:00 am" < "11:00 am" evaluates to true

For this reason, it would be better to ensure that the function always returns values in a consistent format.

@cjyuan cjyuan removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 8, 2026
@chefmatthaus-hub chefmatthaus-hub added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 10, 2026

@cjyuan cjyuan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Changes look good.

@cjyuan cjyuan added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Reviewed Volunteer to add when completing a review with trainee action still to take. labels Oct 10, 2026
@cjyuan

cjyuan commented Oct 10, 2026

Copy link
Copy Markdown
Contributor

Note: The Changelist section is missing a "Changelist" heading.

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

Labels

Complete Volunteer to add when work is complete and all review comments have been addressed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants