-
-
Notifications
You must be signed in to change notification settings - Fork 219
Birmingham | ITP-Sep-25 | Ahmad Ehsas | sprint 2 | Data groups #805
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…rray and passed the tests of given arrays.
…seNumber property.
…by join(\n) method.
|
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
|
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). |
|
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). |
|
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). |
|
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
|
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). |
Can you address these issues? |
|
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
|
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). |
Sprint-2/debug/author.js
Outdated
| for (const key in author) { | ||
| if (author.hasOwnProperty(key)) { | ||
| // if (author.hasOwnProperty(key)) { | ||
| console.log(`${key}: ${author[key]}`); | ||
| } | ||
| } | ||
| //} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
Sprint-2/implement/contains.test.js
Outdated
| 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); | ||
| }); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Sprint-2/interpret/invert.js
Outdated
| // 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'} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
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
left a comment
There was a problem hiding this 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.
| 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); | ||
| }); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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".
Sprint-2/interpret/invert.js
Outdated
| // 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'} | ||
|
|
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this 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.

Self checklist
Changelist
I have completed all the required tasks.
Questions