search

google / mathsteps

Step by step math solutions for everyone
https://socratic.org
Apache License 2.0
2.12k stars 274 forks source link

Use provided cloneDeep() #213

Closed ericluap closed 6 years ago

ericluap commented 7 years ago

There's a function implemented here that does a deep clone but mathjs provides one, 'node.cloneDeep()'. When I replaced the clone function definition with just cloneDeep all of the tests passed. There might have been a reason that cloneDeep() was not chosen to be used, but it seems to be all fixed now. Should we switch to cloneDeep()?

evykassirer commented 7 years ago

aha! https://github.com/josdejong/mathjs/issues/745#issuecomment-261547371

So there were issues, but they were fixed. We just delayed changing to use their new stuff. Feel free to make the change to use their cloneDeep now :)

abeledovictor commented 6 years ago

As a way to start contributing, I want to make the switch to cloneDeep(). Do you think I should change the utils/clone function, or should I replace all places where clone(node) is used, and change it to: node.cloneDeep()? @evykassirer

evykassirer commented 6 years ago

go for it! you might need to update mathjs (not sure)

I think replacing the places where clone(node) is used would be better, but changing the clone function as a first change is also fine if you want to do something smaller :)