Rules Test

[aliang8]

Created rules.js and rules_test.js.

I'm not sure if that is the way you want to me to export the rules.

Also, I commented out some of the test cases because of this:

One last note is that the parentheses aren't being parsed/ recognized. Is that true?

[kevinbarabash]

I had a look into the test failures. It's because math-parser parses -2 as

{ type: 'Number', value: '-2' }

whereas -#a is parsed as

{ type: 'Operation', op: 'neg', args: [ { type: 'Placeholder', name: 'a' } ] }

This throws off the matchNode because it's trying to match the same structure. I'm going to change how we parse negative numbers. I'm tracking that work in https://github.com/semantic-math/math-parser/issues/33.

[aliang8]

So, math-parser would have to parse -2 as

{ type: 'Operation', op: 'neg', args: [ { type: Number', name: '2' } ] }

[kevinbarabash]

So, math-parser would have to parse -2 as

{ type: 'Operation', op: 'neg', args: [ { type: Number', name: '2' } ] }

Yes, that's right. I've made the change in math-parser published a new version to npm. You'll have to do yarn update math-parser to get the change.

[aliang8]

I upgraded my math-parser, but the same error is still showing up.

[kevinbarabash]

I upgraded my math-parser, but the same error is still showing up.

I upgrade math-parser as well and wrote the following test which passes:

        it.only('cancel minuses', () => {
            const rule = defineRuleString('-#a / -#b', '#a / #b');
            assert.equal(applyRuleString(rule, '-2 / -1'), '2 / 1');
        });

Can you push your changes after upgrading?

[aliang8]

Yup, will do in a little bit. Sorry for the delay.

[kevinbarabash]

It looks like yarn update math-parser didn't actually update package.json like it should've. My guess is that yarn update was run too soon after I published. Try it again. It should change the version to ^0.2.2 in the package.json and update yarn.lock as well.

[aliang8]

I think the command is yarn upgrade math-parser. I tried running that command and retesting but it didn't work. I also tried to reclone the math-parser repo, but that failed also. I pushed most of the test cases and left the null error one there for you to see.

[kevinbarabash]

I had a quick look at the test cases and they're looking great.

Yes yarn upgrade math-parser is the right command. Can you paste the output you're seeing? Here's what I'm seeing:

yarn upgrade v0.19.1
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] 🔗  Linking dependencies...
[4/4] 📃  Building fresh packages...
success Saved 1 new dependency.
└─ math-parser@0.2.2
✨  Done in 2.30s.

[aliang8]

Yup. Also are there any specific changes I should be looking for in math-parser.

[kevinbarabash]

@aliang8 which folder are you running this in?

[aliang8]

I ran yarn upgrade math-parser inside my math-parser folder.

UPDATE: i see the mistake (was suppose to run it in math-rules), but I don't understand why. :)

[kevinbarabash]

Sorry... I should've been more specific. You'll need to run yarn upgrade math-parser in the math-rules folder b/c math-parser is a dependency of math-rules.

[aliang8]

I see! All good. I will finish up with the tests.

[kevinbarabash]

Yup. Also are there any specific changes I should be looking for in math-parser.

The only change I've published recently has been the change to how negative numbers are parsed. I'm working right now on a change to merge 'Function', 'Operation', and 'Relation' nodes into 'Apply' nodes. I haven't published that yet, but I don't foresee that affect this diff (fingers crossed).

The only time you'll need to touch your local copy of math-parser is if you need to make changes to the parser. I'm publishing changes to math-parser as I make them but you don't have publish rights so we can chat about how to test changes across repos locally without publishing when the need arises.

[aliang8]

I'm working right now on a change to merge 'Function', 'Operation', and 'Relation' nodes into 'Apply' nodes.

Awesome stuff! Just one last issue with my tests. The ones I had to skip are giving me a weird testing error. Would you mind taking a look at it?

Also, I will try to add a linter based on your preferences to math-rules in a separate PR. Just a heads-up.

[kevinbarabash]

The ones I had to skip are giving me a weird testing error. Would you mind taking a look at it?

You a test with the same name remove multiplying by negative one twice. Test names must be unique or weird stuff will happen.

[aliang8]

Good catch! It still didn't fix the error though.

[kevinbarabash]

@aliang8 I had a look at the issue. The problem is that populatePattern wasn't clone the pattern template before populating it with nodes from the placeholders argument. Here's the fix:

--- a/lib/matcher.js
+++ b/lib/matcher.js
@@ -150,7 +150,7 @@ const checkBounds = (indexes, array) =>
     indexes.start > 0 || indexes.end < array.length - 1

 export const populatePattern = (pattern, placeholders) => {
-    return replace(pattern, {
+    return replace(clone(pattern), {
         leave(node) {
             if (node.type === 'Placeholder' && node.name in placeholders) {
                 return clone(placeholders[node.name])

Can you make that change as part of you diff?

We'll want to write some tests for these individual functions to that verify that the functions don't modify the arguments, but that can be a separate diff.

[kevinbarabash]

I've updated math-rules to use the versions of math-parser and math-traverse which use Apply nodes in place of Function, Operation, and Relation nodes. Let me know if you run into any issues rebasing.

[aliang8]

Awesome, that did the trick! I will try to understand why tomorrow.

For tonight, I think I will finish the test cases, take a look at the issues, and call it a day.

[aliang8]

Hey Kevin, I just finished adding the rest of the cases. Some things that I noticed:

• [FIXED] The SIMPLIFY SIGNS case still returns a node error.
• Extra parens or lack thereof e.g CANCEL_MINUSES [ '-(x + 1) / -1' , '((x + 1)) / 1' ] <- extra paren MULTIPLY_BY_INVERSE [ '2 / (3 / 4)' , '2 * 4 / 3' ] <- loses paren

[kevinbarabash]

I ran the following code:

const rule = defineRuleString('-#a / -1', '#a / 1');
const ast = parse('-(x + 1) / -1');
const outAST = applyRule(rule, ast);
console.log(JSON.stringify(outAST, null, 4));

and got the following output AST:

{
    "type": "Apply",
    "op": "div",
    "args": [
        {
            "type": "Apply",
            "op": "add",
            "args": [
                {
                    "type": "Identifier",
                    "name": "x"
                },
                {
                    "type": "Number",
                    "value": "1"
                }
            ]
        },
        {
            "type": "Number",
            "value": "1"
        }
    ]
}

which leads me to believe that the issue is with print in math-parser, as opposed to the parse function.

[aliang8]

Right, that might in fact be the issue. Does that also explain the case where the solution is missing parentheses?

UPDATE: I fixed the SIMPLIFY_SIGN test. Nothing wrong with the parser or print there.

[kevinbarabash]

I think [ '2 / (3 / 4)' , '2 * 4 / 3' ] is fine. Although there's parens in 2 / (3 / 4), the parser doesn't include them in the output because it can describe the order of operations with Apply:div nodes only. Printing out that AST should result in 2 / (3 / 4). The output string is correct b/c the parser parses 2 * 4 / 3 as 2 * (4/3).

[kevinbarabash]

I'm going to update math-parser with the fix for the extra parens issue.

[aliang8]

Awesome! It all makes sense now. That was why the parser was structured in that way, order of operations. 👍

[kevinbarabash]

Okay, I've pushed and published the changes to math-parser. Sorry about the changes I pushed to math-rules. You'll have to merge again and the update the math-parser dependency using yarn and then the tests should pass.

[kevinbarabash]

Yay! 🎉 That was a bit of a slog but it uncovered a number of issues which got fixed. 👍