[LEMS-2777] Bugfix: KAS handling functions around plain numbers #2266
Conversation
handeyeco
changed the title
|
Size Change: +149 B (+0.02%) Total Size: 871 kB
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (8dcc700) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR2266If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR2266 |
handeyeco
requested review from
jeremywiebe,
benchristel,
kevinb-khan and
a team
packages/kas/src/nodes.ts
Outdated
| // if no variables, only need to evaluate once | ||
| if (!varList.length && !this.has(Unit) && !other.has(Unit)) { | ||
| return equalNumbers(this.eval(), other.eval()); | ||
| } |
There was a problem hiding this comment.
I have very low confidence that this change won't introduce a bug. KAS has always been very tricky to me. However all new and old tests are passing, I'm going to encourage a thorough QE run, and I would love if I could get a thoughts/feels from other devs.
There was a problem hiding this comment.
As an alternative, this also passes tests:
const varAndFuncList = _.union(
this.getVars(/* excludeFunc */ false),
other.getVars(/* excludeFunc */ false),
);
// if no variables, only need to evaluate once
if (!varAndFuncList.length && !this.has(Unit) && !other.has(Unit)) {
return equalNumbers(this.eval(), other.eval());
}
There was a problem hiding this comment.
From the comment it makes me think that this was an optimization to avoid unnecessary computation. Out of curiosity, did this return statement ever get hit when running the tests?
There was a problem hiding this comment.
I think the alternative you've proposed makes sense.
There was a problem hiding this comment.
Out of curiosity, did this return statement ever get hit when running the tests?
I tested it out and it seems to hit that return statement in the "evaluate only" tests. I think it was an optimization for cases like expect("2+2").toEqualExpr("4");
packages/kas/src/nodes.ts
Outdated
| // if no variables, only need to evaluate once | ||
| if (!varList.length && !this.has(Unit) && !other.has(Unit)) { | ||
| return equalNumbers(this.eval(), other.eval()); | ||
| } |
There was a problem hiding this comment.
From the comment it makes me think that this was an optimization to avoid unnecessary computation. Out of curiosity, did this return statement ever get hit when running the tests?
packages/kas/src/nodes.ts
Outdated
| // if no variables, only need to evaluate once | ||
| if (!varList.length && !this.has(Unit) && !other.has(Unit)) { | ||
| return equalNumbers(this.eval(), other.eval()); | ||
| } |
There was a problem hiding this comment.
I think the alternative you've proposed makes sense.
Summary:
See the ticket for the full break-down of the issue.
What I think was happening:
f(2)fis a function)f(2)like it was a RationalI just removed the part of the code that was trying to take a shortcut.
Issue: LEMS-2777
Test plan: