Skip to content

Conversation

fruddenfeldt
Copy link
Contributor

@fruddenfeldt fruddenfeldt commented May 22, 2023

Because

The test for the 'multiply' function used array brackets [] for the input parameters:

expect(calculator.multiply([2,4])).toBe(8);

This causes the test to return an error when rest parameters (...args) are used in the function, like so:

const multiply = function(...args){
  return args.reduce((acc, cur) => acc * cur);
}

This PR

  • removed brackets on line 43 (the test for multiplying two values)
  • removed brackets on line 47 (the test for multiplying many values)

Issue

Closes #XXXXX

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 changes that needs to be updated on the solutions branch, I have created another PR (and linked it to this PR).

The test for the 'multiply' function used array brackets [] for the input parameters, which caused the test to return an error when rest parameters (...args) are used in the function, like so:

const multiply = function(...args){
  return args.reduce((acc, cur) => acc * cur);
}
@codyloyd
Copy link
Member

codyloyd commented Jun 5, 2023

This is a good idea I think.

Can you also do a PR to the solutions branch? the solution we have is geared toward these tests, and expects an array as a parameter.. but none of the other base-math functions (add, subtract, etc.) expect an array, so it makes sense to be consistent here.

@codyloyd
Copy link
Member

codyloyd commented Jun 5, 2023

I don't want to merge this one until the solutions are updated... so let me know when that PR is ready

fruddenfeldt added a commit to fruddenfeldt/javascript-exercises that referenced this pull request Jun 6, 2023
See separate PR "Removed array syntax in multiply test TheOdinProject#359"  

TheOdinProject#359
@fruddenfeldt
Copy link
Contributor Author

Thanks. Done - see this PR: #364

@codyloyd codyloyd merged commit 6b302e3 into TheOdinProject:main Jun 7, 2023
@cats256
Copy link
Contributor

cats256 commented Jul 4, 2023

This is a good idea I think.

Can you also do a PR to the solutions branch? the solution we have is geared toward these tests, and expects an array as a parameter.. but none of the other base-math functions (add, subtract, etc.) expect an array, so it makes sense to be consistent here.

If that's the case, we should also modify the test for the sum function in calculator.spec.js to expect rest parameters instead of expecting an array parameter. The same thing applies to the calculator-solution.spec.js file, in which the test for sum and multiply functions expects an array instead of rest parameters, causing it to fail two tests.

@cats256
Copy link
Contributor

cats256 commented Jul 4, 2023

I opened a PR

Oussama5379 added a commit to Oussama5379/javascript-exercises that referenced this pull request Feb 1, 2025
See separate PR "Removed array syntax in multiply test TheOdinProject#359"  

TheOdinProject#359
Oussama5379 added a commit to Oussama5379/javascript-exercises that referenced this pull request Feb 1, 2025
Removed array syntax in multiply test
Clovena pushed a commit to Clovena/odin-foundations that referenced this pull request Mar 2, 2025
See separate PR "Removed array syntax in multiply test #359"  

TheOdinProject/javascript-exercises#359
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.

3 participants