Skip to content

Manchester | 26-ITP-Sept | Fatima Rouchi | Sprint 2 | Implement and rewrite tests - #1728

Open
fatimarouchi wants to merge 24 commits into
CodeYourFuture:mainfrom
fatimarouchi:coursework/sprint-3-implement-and-rewrite
Open

Manchester | 26-ITP-Sept | Fatima Rouchi | Sprint 2 | Implement and rewrite tests#1728
fatimarouchi wants to merge 24 commits into
CodeYourFuture:mainfrom
fatimarouchi:coursework/sprint-3-implement-and-rewrite

Conversation

@fatimarouchi

@fatimarouchi fatimarouchi commented Oct 17, 2026 •
edited
Loading

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

CYF-1059

Changelist

  • Implemented angle classification and added tests for valid and invalid angles.
  • Corrected proper-fraction logic and added tests for positive and negative values.
  • Implemented card-value validation and added tests for valid cards and invalid formats.

@fatimarouchi fatimarouchi changed the title Manchester | 26-ITP-Sept | Fatima Rouchi| Sprint 2 | Implement and rewrite tests Manchester | 26-ITP-Sept | Fatima Rouchi | Sprint 2 | Implement and rewrite tests Oct 17, 2026
@fatimarouchi fatimarouchi added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 17, 2026
@cjyuan cjyuan added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Oct 23, 2026
cjyuan
cjyuan reviewed Oct 23, 2026
View reviewed changes
Comment thread implement-and-rewrite-tests/implement/3-get-card-value.js Resolved
Comment thread implement-and-rewrite-tests/rewrite-tests-with-jest/1-get-angle-type.test.js Resolved
Comment thread implement-and-rewrite-tests/rewrite-tests-with-jest/2-is-proper-fraction.test.js Resolved
Comment thread implement-and-rewrite-tests/rewrite-tests-with-jest/3-get-card-value.test.js Resolved
Comment thread implement-and-rewrite-tests/rewrite-tests-with-jest/3-get-card-value.test.js Resolved
Comment thread implement-and-rewrite-tests/implement-tests-with-node-test/1-get-angle-type.test.js Resolved
@cjyuan cjyuan added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Oct 23, 2026
@fatimarouchi fatimarouchi added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Oct 26, 2026
@cjyuan

cjyuan commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

It seems some of the inline comments were not yet addressed. Could you address all the inline comments in addition to making changes?

Suggestion: Use AI to explore "How to respond to inline comments in a PR"

@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 27, 2026
@cjyuan

cjyuan commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

Also, the PR description is missing a Changelist section.

Could you also use AI to explore "what to write in the Changelist section of a PR?"

@fatimarouchi fatimarouchi added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Oct 27, 2026
cjyuan
cjyuan reviewed Oct 27, 2026
View reviewed changes

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

  • The implementation of isProperFraction() and its Jest tests are not fully compatible.

  • Your PR description is missing a "Changelist" section.

Comment thread implement-and-rewrite-tests/rewrite-tests-with-jest/2-is-proper-fraction.test.js Resolved
Comment thread implement-and-rewrite-tests/rewrite-tests-with-jest/3-get-card-value.test.js Resolved
@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 27, 2026
@fatimarouchi fatimarouchi added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Nov 3, 2026
cjyuan
cjyuan reviewed Nov 3, 2026
View reviewed changes

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

Comment thread implement-and-rewrite-tests/implement-tests-with-node-test/2-is-proper-fraction.test.js

@cjyuan cjyuan Nov 3, 2026

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.

The tests in this script are comprehensive. Well done!

Note: The tests in the Jest test script could also be categorised in similar fashion.

@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. labels Nov 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjyuan cjyuan cjyuan left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@fatimarouchi @cjyuan