Skip to content

Conversation

@ahmadehsas
Copy link

@ahmadehsas ahmadehsas commented Oct 28, 2025

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

I have completed all the required tasks.

Questions

@github-actions
Copy link

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Sprint part (Data Groups) doesn't match expected format (example: 'Sprint 2', without quotes)

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

3 similar comments
@github-actions
Copy link

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Sprint part (Data Groups) doesn't match expected format (example: 'Sprint 2', without quotes)

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

@github-actions
Copy link

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Sprint part (Data Groups) doesn't match expected format (example: 'Sprint 2', without quotes)

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

@github-actions
Copy link

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Sprint part (Data Groups) doesn't match expected format (example: 'Sprint 2', without quotes)

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

@ahmadehsas ahmadehsas added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 28, 2025
@github-actions
Copy link

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Sprint part (Data Groups) doesn't match expected format (example: 'Sprint 2', without quotes)

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

1 similar comment
@github-actions
Copy link

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Sprint part (Data Groups) doesn't match expected format (example: 'Sprint 2', without quotes)

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

@cjyuan
Copy link
Contributor

cjyuan commented Nov 11, 2025

  • Title is not in the right format.

  • PR branch is not clean.

Can you address these issues?

@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 Nov 11, 2025
@github-actions
Copy link

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Sprint part (Data Groups) doesn't match expected format (example: 'Sprint 2', without quotes)

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

1 similar comment
@github-actions
Copy link

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Sprint part (Data Groups) doesn't match expected format (example: 'Sprint 2', without quotes)

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

@ahmadehsas ahmadehsas changed the title West Midlands | ITP-Sep-25 | Ahmad Ehsas | Data Groups | Sprint-2 Birminham | ITP-Sep-25 | Ahmad Ehsas | Data Groups | Sprint-2 Nov 12, 2025
@ahmadehsas ahmadehsas changed the title Birminham | ITP-Sep-25 | Ahmad Ehsas | Data Groups | Sprint-2 Birmingham | ITP-Sep-25 | Ahmad Ehsas | Data Groups | Sprint-2 Nov 12, 2025
@ahmadehsas ahmadehsas 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 23, 2025
Comment on lines 20 to 24
for (const key in author) {
if (author.hasOwnProperty(key)) {
// if (author.hasOwnProperty(key)) {
console.log(`${key}: ${author[key]}`);
}
}
//}
Copy link
Contributor

Choose a reason for hiding this comment

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

Leaving commented out unused code can make code review difficult. Can you keep the the code clean by removing all unnecessary code?
image

Copy link
Author

Choose a reason for hiding this comment

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

I removed the unnecessary code.

Comment on lines 48 to 56
test("contains on an array of length returns true", () => {
expect(contains([1, 2, 3, 4], "length")).toBe(true);
// expect(contains([1, 2, 3, 4], "length")).toBe(true);
expect(contains(null, "a", "length")).toBe(true);
}); // the test returns true because 'length' is a property of the array object

test("contains given on array as input return false", () => {
expect(contains([1, 2, 3], "a")).toBe(false);
// expect(contains([1, 2, 3], "a")).toBe(false);
expect(contains(1234, "a")).toBe(false);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you expect the function to accept an array as its first parameter or not? These two test descriptions are contradicting each other.

Copy link
Author

Choose a reason for hiding this comment

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

The descriptions were unclear; the function does accept an array as the first parameter. I have now updated the descriptions.
I do apologise for my emojis; I just wanted to ensure I am looking for a solution to this review.

Comment on lines 23 to 26
// The current return value is {'1': 'a'}

// b) What is the current return value when invert is called with { a: 1, b: 2 }
// The current return value is {1: "a", 2: "b"}
// The current return value is {'1': 'a', '2': 'b'}
Copy link
Contributor

Choose a reason for hiding this comment

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

These are still not the objects returned by the original (unmodified) function.

Copy link
Author

Choose a reason for hiding this comment

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

I have updated my code and answers

@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 Nov 23, 2025
@ahmadehsas ahmadehsas 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 24, 2025
@cjyuan
Copy link
Contributor

cjyuan commented Nov 25, 2025

On some of the issues I commented, I don't see the changes I expected, and I am not good at guessing what the emoji reaction means. Can you respond to each of my comments with words?

@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 Nov 25, 2025
@ahmadehsas ahmadehsas 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 25, 2025
Copy link
Contributor

@cjyuan cjyuan left a comment

Choose a reason for hiding this comment

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

You can use emojis, just not as a replacements for clear responses.

Comment on lines 48 to 54
test("contains on array with existing property returns true", () => {
expect(contains([1, 2, 3, 4], "length")).toBe(true);
});

test("contains on array with non-existent property returns false", () => {
expect(contains(1234, "a")).toBe(false);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you check carefully what could be improved?

Copy link
Author

Choose a reason for hiding this comment

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

Missing Given /When / Then style.

Copy link
Contributor

Choose a reason for hiding this comment

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

You have all the elements in the style your mentioned. So even without those words, your description still fit the style.

I was referring to something else. Is its something that you should "fix".

Comment on lines 33 to 37
// The current return value is {'1': 'a'}

// b) What is the current return value when invert is called with { a: 1, b: 2 }
// The current return value is {'1': 'a', '2': 'b'}

Copy link
Contributor

Choose a reason for hiding this comment

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

"Current return value" (in questions (a), (b), (d)) refers to the object returned by the original (unmodified) function -- the function before you modified it.

Can you update the answers to these three question accordingly?

@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 Nov 25, 2025
@ahmadehsas ahmadehsas 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 26, 2025
Copy link
Contributor

@cjyuan cjyuan left a comment

Choose a reason for hiding this comment

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

Changes are good. Would be better if you also responded to each comment.

@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 27, 2025
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. Module-Data-Groups The name of the module. 📅 Sprint 2 Assigned during Sprint 2 of this module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants