merge should assert that it has at least one source

phetsims / phet-core

Core utilities used by all PhET simulations.

MIT License

8 stars 6 forks source link

merge should assert that it has at least one source #70

Closed zepumph closed 4 years ago

zepumph commented 4 years ago

Today in a design meeting, @jbphet and @samreid said a bug they have run into with _.extends is forgetting to add the "options" object as the second argument, like:

options = _.extend( {
. . . 
} );

_.extend doesn't treat this as an issue, but we think that merge should. I'll add the assertion.

zepumph commented 4 years ago

I added the assertion above. @jbphet will you please review. I also added a test for it (which is passing with the rest of them), and found that vector-addition, wave-interference, and gravity-force-lab all run (chosen based on current usages of merge in the project.

jbphet commented 4 years ago

The added assertion looks reasonable to me. The commit appears to include some other changes to the format of the closures in mergeTests.js. I think these look okay too, but I haven't used assert.throws explicitly myself, so I'm assigning back to @zepumph (as opposed to just closing) in case he wants more scrutiny on this part. He can close at his discretion if he's confident on that part.