search

google / mathsteps

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

Porting math-rules #164

Closed aliang8 closed 7 years ago

aliang8 commented 7 years ago

Porting rules from math-rules to be used in mathsteps.

evykassirer commented 7 years ago

Travis is saying the linter and tests are failing - do you know what's up with that?

kevinbarabash commented 7 years ago

The tests are comparing the printed math string against one another so we should be able to use these tests. I see great benefit in keeping these tests here b/c it means that we'll be able to show that we haven't broken anything or changed the behavior. I worry about tests going missing or changing when being moved over to math-rules.

I would start by disabling all of the tests and then enable the simplest test we can find and trying to get that test passing.

kevinbarabash commented 7 years ago

Also, it may make more sense to define some of the more complicated rules using defineRule in the mathsteps codebase. My hope with is that math-rules will be a set of tools for building rewrite rules with some examples as opposed store house for all rewrite rules that anyone might want.

Lastly, I don't think having a single PR to port mathsteps to use math-rules is wise. It will result in a massive PR that will take a long time to review. Instead, I'd suggest having a porting branch and making PRs against that branch. That way there can be multiple smaller PRs that are easier to reason about and quicker to review.

evykassirer commented 7 years ago

Thanks Kevin. Yeah I was waiting for this PR to get to a reviewable state, and smaller reviewable PRs would be ideal.

Also, I was assuming all our rules would be defined here and not by math-rules?

aliang8 commented 7 years ago

If I understand correctly, Kevin wants math-rules to store only the very basic rules that can be used as a foundation to build more complex rules which will be stored in math-steps. That's fine by me, but in regards to the test cases, I feel like having them in both places seems redundant. I don't see why the behavior or test results would change because we will still be using math-rules to test.

kevinbarabash commented 7 years ago

Since mathsteps is the one using the rules, it probably makes more sense to define them here. math-rules only needs a couple of rules to exercise its different capabilities and really those are only needed in the test suites. I'm okay with math-rules only exporting functions like defineRule, canApplyRule, applyRule, etc.

kevinbarabash commented 7 years ago

I've started a branch called math-rules on my fork of mathsteps to show how one might go about integrating math-rules over multiple commits/PRs. Have a look at https://github.com/kevinbarabash/mathsteps/commit/acb1772d6ce01c58386a981e6aabea910ebb57a0.

It disables all tests except for the tests that are actually passing. There were also a number of tests were code was running inside a describe block. This is not good b/c that code will run even if when using describe.skip. All test code should go inside it blocks.

I then updated removeAdditionOfZero.js and enable its tests, see https://github.com/kevinbarabash/mathsteps/commit/acb1772d6ce01c58386a981e6aabea910ebb57a0#diff-0ef865aead473d8a6f2227ab990902dc. The tests for removeAdditionOfZero remain unchanged.

evykassirer commented 7 years ago

closing since we started the porting branch, feel free to reopen if you think we need it open