google / mathsteps

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

Fixes for Kevin's Init PR #176

Closed aliang8 closed 7 years ago

aliang8 commented 7 years ago

updates from code review to #167

deleted some comments removed isPolynomialTerm because it exists now in math-nodes

evykassirer commented 7 years ago

you said it was ready, so here are remaining comment to be addressed:

I'm not gonna keep going cause I'm pretty much copy pasting every comment :p so if you could look at all the comments in https://github.com/socraticorg/mathsteps/pull/167/files and make sure all the comments have been addressed (you can continue the conversation there) that'd be great thanks!

kevinbarabash commented 7 years ago

@evykassirer @aliang8 I've updated math-nodes to do arity checks on most of the operators, see https://github.com/semantic-math/math-nodes/commit/131d5c36fc1cbc88e699eef060a078d8f130976d. I was thinking of adding checks for mul and add to make sure args.length > 1. Thoughts?

evykassirer commented 7 years ago

@kevinbarabash I like that!