search

google / mathsteps

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

Start of porting mathsteps to use math-rules #167

Closed kevinbarabash closed 7 years ago

kevinbarabash commented 7 years ago

This PR disables almost all of the existing tests except for the ones that are currently passing after being ported. Note that the tests themselves have not been changed.

The plan build upon this foundation with a series of small PRs to port each of the various search commands and then eventually update the simplifyExpression and solveEquation.

aliang8 commented 7 years ago

Thanks for the comments! I will look over this PR soon.

aliang8 commented 7 years ago

Great! This PR looks good to merge. Thanks for leading the way Kevin!

kevinbarabash commented 7 years ago

Great! This PR looks good to merge. Thanks for leading the way Kevin!

Please don't merge it yet. There's still some unresolved issues that I'd like to take care of first.

aliang8 commented 7 years ago

I wasn't going to haha, sorry if I was a bit rushed in saying that.

kevinbarabash commented 7 years ago

Sorry for making this diff so big. I want to make sure simplifyExpression was working for at least a couple of situations. I wanted to minimize the changes to simplify.js and stepThrough.js so I ended up having to update removeUnnecessaryParens and flattenOperands while making sure their tests pass.

aliang8 commented 7 years ago

All good. I'll take a look at this PR tomorrow at work!

kevinbarabash commented 7 years ago

There are 40 passing tests and 462 pending.

kevinbarabash commented 7 years ago

You may find reviewing each commit easier since they're more self contained.

aliang8 commented 7 years ago

Kevin, can you fix the linter issue? Then I can merge the PR and start porting other rules.

evykassirer commented 7 years ago

would it be okay for me to check out this PR before merging? I could probably look tomorrow

or since it's still on a feature branch I could just look after merging if you want to get started on stuff soon

evykassirer commented 7 years ago

sorry I've been so inactive - I've been wanting to check this out for a while! :)

finally figured out my internship plans for next term and have a bit more time now

kevinbarabash commented 7 years ago

I think the reason why the pre-commit hook for lint isn't working is that it makes some assumptions that aren't always true:

"setup-hooks": "ln -s ../../scripts/git-hooks/pre-commit.sh .git/hooks/pre-commit"

I've been quite pleased with https://www.npmjs.com/package/pre-commit. Maybe we can use it here as well.

evykassirer commented 7 years ago

wow kevin this is amazing I'm so excited

thanks for all the work you've been putting into this!!

I gotta do some other things before I sleep, but I'll try to reply to your stuff tomorrow or latest the weekend. Excited to be thinking about these things again!

kevinbarabash commented 7 years ago

Eek... I guess I'll work all the feedback into a followup PR.

evykassirer commented 7 years ago

yeah sorry I would have preferred this stay open too, but it was hard for Anthony to work off of your stuff because it was in another fork (maybe we can make you a contributor, or there's some other better solution, idk)

evykassirer commented 7 years ago

we could also reopen this PR, revert the change, and anthony can rebase later :p

kevinbarabash commented 7 years ago

we could also reopen this PR, revert the change, and anthony can rebase later :p

I don't want to hold things up. Maybe @aliang8 could do some of changes we've identified as wanting to make.

aliang8 commented 7 years ago

Yup, of course. I'll make the changes Evy requested.

Meanwhile, I restarted porting the basic rules. Opened a new PR, let me know how it looks.