Skip to content

05_sumAll: Update README.md #475

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

Merged
merged 4 commits into from
May 25, 2024

Conversation

shubhatRashid
Copy link
Contributor

@shubhatRashid shubhatRashid commented May 25, 2024

Because

The test cases for the sumAll question suggested that the integers needed to be positive however no such thing was mentioned in question statement .

This PR

Changes the question statement which previously stated to 'form a function for integers' to 'form a function for positive integers'

Issue

Closes #255

Additional Information

Pull Request Requirements

  • I have thoroughly read and understand The Odin Project Contributing Guide
  • The title of this PR follows the location of change: brief description of change format, e.g. 01_helloWorld: Update test cases
  • The Because section summarizes the reason for this PR
  • The This PR section has a bullet point list describing the changes in this PR
  • If this PR addresses an open issue, it is linked in the Issue section
  • If this PR includes any changes that affect the solution of an exercise, I've also updated the solution in the /solutions folder

Copy link
Contributor

@MaoShizhong MaoShizhong left a comment

Choose a reason for hiding this comment

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

Thanks @shubhatRashid. In the future, could you please comment on the relevant issue and request to be assigned? Then only open a PR after you've been assigned by a maintainer as per our contributing guide.

Just need a quick spacing fix as well as removing the two unrelated changes that accidentally made their way into this commit.

Comment on lines 1 to 2
const removeFromArray = function(...arg) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Reverting unrelated file to original state

Suggested change
const removeFromArray = function(...arg) {
const removeFromArray = function() {

Comment on lines 16 to 17
test('ignores non present values, but still works', () => {
expect(removeFromArray([1,2, 2, 3, 4], 7, 2)).toEqual([1, 3, 4]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Reverting unrelated file to original state

Suggested change
test('ignores non present values, but still works', () => {
expect(removeFromArray([1,2, 2, 3, 4], 7, 2)).toEqual([1, 3, 4]);
test.skip('ignores non present values, but still works', () => {
expect(removeFromArray([1, 2, 3, 4], 7, 2)).toEqual([1, 3, 4]);

@@ -1,6 +1,6 @@
# Exercise 05 - sumAll

Implement a function that takes 2 integers and returns the sum of every number between(and including) them:
Implement a function that takes 2 positive integers and returns the sum of every number between(and including) them:
Copy link
Contributor

Choose a reason for hiding this comment

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

Quick fix

Suggested change
Implement a function that takes 2 positive integers and returns the sum of every number between(and including) them:
Implement a function that takes 2 positive integers and returns the sum of every number between (and including) them:

@shubhatRashid
Copy link
Contributor Author

Hey @MaoShizhong ,
I will keep the guidelines under consideration next time, i have made the requested changes .

Copy link
Contributor

@MaoShizhong MaoShizhong left a comment

Choose a reason for hiding this comment

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

The following bits are still necessary to remove the unrelated files from the PR

@@ -1,5 +1,5 @@
const removeFromArray = function() {

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

@@ -14,7 +14,7 @@ describe('removeFromArray', () => {
expect(removeFromArray([1, 2, 3, 4], 7, "tacos")).toEqual([1, 2, 3, 4]);
});
test.skip('ignores non present values, but still works', () => {
expect(removeFromArray([1, 2, 3, 4], 7, 2)).toEqual([1, 3, 4]);
expect(removeFromArray([1,2, 3, 4], 7, 2)).toEqual([1, 3, 4]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(removeFromArray([1,2, 3, 4], 7, 2)).toEqual([1, 3, 4]);
expect(removeFromArray([1, 2, 3, 4], 7, 2)).toEqual([1, 3, 4]);

@shubhatRashid
Copy link
Contributor Author

Hey @MaoShizhong ,
Sorry for my silly mistakes,
i have made the necessary changes.

Copy link
Contributor

@MaoShizhong MaoShizhong left a comment

Choose a reason for hiding this comment

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

Many thanks!

@MaoShizhong MaoShizhong merged commit 312bd8a into TheOdinProject:main May 25, 2024
@shubhatRashid
Copy link
Contributor Author

My pleasure...

Oussama5379 added a commit to Oussama5379/javascript-exercises that referenced this pull request Feb 1, 2025
* Change 'integers' to 'positive integers' in question statement

* requested changes

* requested changes

* requested changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug - : 05_sumALL: Returning ERROR with negative numbers
2 participants