google / mathsteps

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

Should `print` add in parens automatically where necessary? #126

Closed kevinbarabash closed 7 years ago

kevinbarabash commented 7 years ago

Having to add explicit ParenthesesNodes makes the expression tree harder to manipulate and may result in output which doesn't look like what the expression tree represents.

Right now print({ "/" [ { "+" [ "x", "2" ] } "2" }) => "x + 2 / 2". Re-parsing "x + 2 / 2" results in { "+" [ "x", { "/" [ "2", "2" ] } ] } which is not the same expression.

I'd like to propose the following behavior change for print:

print({ "/" [ { "+" [ "x", "2" ] } "2" }) => "(x + 2) / 2"
print({ "+" [ { "+" [ "x", "x" ] } { "+" [ "2", "2" ] } ] }) => "(x + x) + (2 + 2)"

The change would update print to add parens around any + operation automatically if it isn't the root node.

evykassirer commented 7 years ago

this seems quite reasonable. would definitely make some things easier (removeUnnecessaryParens would be simpler too)

I'm curious if there'd be any complications that come out of this, but my intuition is that it'd be alright. Thanks for opening this :)

evykassirer commented 7 years ago

thanks for taking this on @Flyr1Q :D

sqrtsanta commented 7 years ago

@evykassirer I'm already working on this, you're welcome ;)

sqrtsanta commented 7 years ago

@evykassirer Ok, I've tried to implement what @kevinbarabash suggested.

The change would update print to add parens around any + operation automatically if it isn't the root node.

print({ "/" [ { "+" [ "x", "2" ] } "2" }) => "(x + 2) / 2" <- perfectly fine

120 is fixed as well

But this is not ok for the print({ "+" [ { "+" [ "x", "x" ] } { "+" [ "2", "2" ] } ] }) => "(x + x) + (2 + 2)"

// As proposed, everything is ok
// expected: x + 4 - (12 + x)
// got: (x + 4) - (12 + x)

// Whoops, looks weird I think..
// expected: 4x^2 + 2x + 7
// got: (4x^2 + 2x) + 7

Then I realized not all nodes, passed to util > print.js, are always flattened. Simplify (unflatten) relies on print as well. I just wanted to ask what do you think about this, since I am not well aware of all the code.

evykassirer commented 7 years ago

cool! I'm glad you're getting somewhere, and also that's an interesting tricky thing :o

you're right - flattening will be needed. good catch. I would say just call flatten on the node before printing it. Does that work?

sqrtsanta commented 7 years ago

@evykassirer Sorry for double log. Noticed I made unnecessary change, recreated branch in own repository. Still do not have much experience in open sourcing :D

evykassirer commented 7 years ago

that's okay! I'll go take a look. I don't have much experience in open source either - so we're both new hehe

btw if you ever need help with git, feel free to ping me on https://gitter.im/mathsteps-chat/Lobby

[EDIT: the double log is totally fine. It's just cause you referenced this issue on both commit's messages. no problem at all!]