Skip to content

London | 26-May-ITP | Yonatan Teklemariam | Sprint 3 | implement and rewrite tests#1452

Open
Yonatanteklemariam wants to merge 21 commits into
CodeYourFuture:mainfrom
Yonatanteklemariam:coursework/sprint-3-implement-and-rewrite-tests
Open

London | 26-May-ITP | Yonatan Teklemariam | Sprint 3 | implement and rewrite tests#1452
Yonatanteklemariam wants to merge 21 commits into
CodeYourFuture:mainfrom
Yonatanteklemariam:coursework/sprint-3-implement-and-rewrite-tests

Conversation

@Yonatanteklemariam

@Yonatanteklemariam Yonatanteklemariam commented Jul 3, 2026

Copy link
Copy Markdown

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

Changelist

removed the redundant checks based on mathematical reference and corrected the syntax errors in the commented out code snippet.

Supersedes and replaced the old PR: #1381

Renamed the branch to better align with PR formatting rules

The code remains exactly the same as the previous PR; only the branch name was updated and some comments added

###Carrying over from #1381. @cjyuan had already reviewed, and advised on removal of redundant checks which is now fixed. Plus he advised me to align to the PR format which is also resolved.

Remove check for zero numerator in isProperFraction function.
Added checks for finite and integer values of numerator and denominator.
Refactor isProperFraction to validate integers instead of finite numbers.
Fix syntax error in isProperFraction function.
@Yonatanteklemariam Yonatanteklemariam added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jul 3, 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 look up

When one must close a reviewed PR and then reopen a new PR from the same codebase, what should they include in the new PR?

@cjyuan

cjyuan commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

I don't see how your implementation can pass your tests. That's why I asked if you had reran your tests after you made changes to make sure everything works as expected.

You missed this in the original 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 Jul 4, 2026
@Yonatanteklemariam

Copy link
Copy Markdown
Author

I don't see how your implementation can pass your tests. That's why I asked if you had reran your tests after you made changes to make sure everything works as expected.

You missed this in the original PR.

Thanks for your patience and constructive feedback @cjyuan. I've re-run the test, identified the failure and corrected it accordingly. Could you please find a moment to review it again and give me your feedback? Thank you.

@Yonatanteklemariam Yonatanteklemariam 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 Jul 4, 2026
Comment on lines +13 to +15
test(`should return false when numerator is zero`, () => {
expect(isProperFraction(0, 1)).toEqual(true);
});

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 test description does not match the test.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

True. I have now fixed it. Could you please review it now? Thanks

@cjyuan cjyuan removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jul 4, 2026
@cjyuan cjyuan added the Reviewed Volunteer to add when completing a review with trainee action still to take. label Jul 4, 2026
It was a typo error and is fixed now.
@Yonatanteklemariam Yonatanteklemariam 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 Jul 5, 2026
@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 Jul 5, 2026
@cjyuan

cjyuan commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Looks good.

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