search

google / mathsteps

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

Node existence check for nthRoot #111

Closed aelnaiem closed 7 years ago

aelnaiem commented 7 years ago

We should check that a node exists when using nthRoot otherwise Cannot read property 'type' of undefined will result from a nonsense query such as x = nthRoot( ), rather than identifying that the query doesn't make sense.

This might be worth discussing with mathjs as a root of nothing might not be something would want to parse as part of a valid math tree.

kevinbarabash commented 7 years ago

Does mathjs not return a parse error when it sees nthRoot( )?

evykassirer commented 7 years ago

it doesn't :(

I believe it's 'cause it parses it as a FunctionNode and those don't have to have arguments. This only becomes an error when you try to evaluate the FunctionNode and it treats it specifically as a root function

@josdejong thoughts?

also a fix here could be in hasUnsupportedNodes.js, make sure that nthroot nodes have exactly 1 or 2 arguments

kevinbarabash commented 7 years ago

Thinking about this some more – not returning a parse error makes sense b/c how many arguments nthRoot requires is a semantic issue. We could do some kind of semantic sanity check on the parse tree. It might also make sense to also check if the argument(s) produces a result within the reals, e.g. nthroot(-1,2).

evykassirer commented 7 years ago

Sounds good! This should all go in hasUnsupportedNodes I think :)

evykassirer commented 7 years ago

(and this would make a great first PR ^^)

shirleymiao commented 7 years ago

https://github.com/socraticorg/mathsteps/pull/121

evykassirer commented 7 years ago

done! thanks @shirleymiao