WIP: Substitute symbols for expressions from optional scope

[Ephs05msm]

This is some work based on a discussion in #209. This happens to be my first ever pull request, so please provide any feedback on things that seem out of sorts. I referenced the contributing guidelines and then followed the instructions from this video to get to this point.

With that said, the approach I ended up taking toward this issue was to:

• Add an optional scope argument to solveEquationString and simplifyExpressionString
• Add a module, Substitute.js, that iterates over a provided scope and replaces symbols with their respective expressions. The intent is to account for nested symbols and the unit test I added to solveEquation.test.js uses such a scenario. I placed the module in ./lib but it likely belongs somewhere else.
• Add a ChangeType, SUBSTITUTE_SCOPE_SYMBOL, that captures instances where symbols are being substituted. I felt this was important to keep the user experience consistent with what it currently is.
• Add a tree search type, substituteScopeSearch, which compares SymbolNodes against the scope. When a match is found, simplifyExpressionString is run (awkwardly due to a TypeError: "...not a function" issue I ran into -- please assist) against the substituted expression and the resulting list of steps (if any) are passed to Node.Status.nodeChanged.

All tests pass, although I only added one additional test to begin to cover this functionality.

I did my best to employ similar patterns to those I found in the codebase as much as I could. There are sure to be some clumsily-handled areas; please provide feedback on those as I am still learning.

Thanks and I look forward to your review, Matt Marino

[Ephs05msm]

Among other mistakes, I accidentally created a test file early on and then never ended up using it. Should probably be removed. Sorry about that.

[evykassirer]

yeah feel free to go through and remove anything not directly related to your PR

I noticed there's also a yarn.lock file which shouldn't be in this PR either - you could add it to the gitignore file!

I think it's getting late for me tonight but I hope to take a look at this in the next couple days :) Thanks for your hard work (it's so cool seeing your learning process and how you do so much without much formal educational background!!!) and for your patience :)

[Ephs05msm]

Sounds good. Couple more commits to get rid of some of the obviously unnecessary stuff. Still a bit of a noob with .gitignore so struggled to get the .vscode folder out. Sorry about that.

Saw a conflicting note regarding yarn.lock so just left it alone for now.

Look forward to your feedback whenever you have time. No rush at all on my end. Thanks again!

[Ephs05msm]

Not directly related to this issue, but I wanted to ask a quick question:

While using this code on another project I came across the following scenario that confused me. I added it to the test suite on my local branch and got the same result:

Same result on master as well (using ['x = 100 + (25^2 / 4)', 'x = 256.25']).

Is it by design that MathSteps won't simplify fractions that could be expressed as a float but not an (unrounded) integer?

[evykassirer]

answered in #215 :)

[Ephs05msm]

Ran into a case where a symbol was nested inside of a UnaryMinus node type and adjusted to account for this. Also added a test to cover. My editor (VS Code) was having end of line sequence issues so fixing those makes it look like I changed a lot of things when in fact only the tests, TreeSearch, and scopeSubstitution search type were meaningfully changed. Sorry about the extra noise. All precommit checks pass.

[evykassirer]

ok so I looked into it and there's no reason for allowUnaryMinus to default to true - I've updated that on master, feel free to pull that in and then your code should work as it was before :)

[Ephs05msm]

Thanks so much for the detailed feedback! I'm all for your suggestions and will get to work on them this week. If I get in over my head anywhere I'll ask for help.

Also, I really appreciate the encouragement. I've found there's a lot of self-doubt along the learning path (at least doing it the way I have been), so even a little bit of affirmation that some things are being done correctly is hugely helpful. Thank you.

On we go :-)

[Ephs05msm]

I replaced the scope arg with options as you suggested, and also attempted to reorder the args so debug is last. Tests pass but it's certainly possible I didn't address it in every place/case. Happy to double back with you if you find issues.

[Ephs05msm]

Thanks again for taking the time to review. I think this latest round of changes get closer to the mark, but there are still some loose ends (especially on the testing side). I felt it better to commit some progress than hold back just to spin my wheels in a few spots. Look forward to continuing to work with you on this :-).

[evykassirer]

ok I think I addressed all your remaining comments/questions!

looks really awesome so far, I'm a fan of the options thing and debug last is 👌 (I am also not sure if you caught them all but seems good!) Thanks for your patience with my review and for working on this new really interesting feature!!

[Ephs05msm]

Thanks, @evykassirer! Planning to spend time on this next week.

[Ephs05msm]

Hi @evykassirer, just wanted to to circle back and let you know that I haven't lost sight of this. Just been focused on a few other things instead. Since this is an isolated branch representing a marginal feature, is it safe to assume that it's alright if I take a break for a bit? If not, I'll make the time.

Thanks again for your help.

[evykassirer]

no worries! I think you're fine, but if the porting change goes through soon and messes you up I'd be happy to help you refactor your stuff (it'd be just a bunch of really small changes) :)