-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow valid permutations of molecules #1
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and nice to see a new feature being added by someone else! Not my initial thought process on how to solve the issue but I think definitely slicker.
It's a shame that atom counting was not as straight forward as I initially assumed.
I don't have time to properly test this so will leave it for proper code review (maybe someone else can learn my eccentric checker) but from the perspective of understanding how the checker works this appears sound.
The only other request I would have is to move the tagging of Potentially rename it |
This set of PRs now also covers a change to recognising matching bracket types, adds/re-prioritises default checker feedback, and adds some better server logging for the sake of being able to diagnose future problems. |
// The if statements signal to the type checker what we already know | ||
switch (node.type) { | ||
case "expr": { | ||
if(isExpression(node)) { | ||
let terms: Term[] = []; | ||
|
||
// Recursively flatten | ||
// Recursively augmentten |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/flat/augment/g
not quite right everywhere 😂
Allows for permutations of molecules to be accepted when the
allowPermutations
flag is set (off by default)e.g.
CH3(CH2)8CH3
orH22C10
forC10H22
and vice-versaNOTE: Only allows for Compound permutations of a Compound, so representing an entire Compound as a Bracket e.g.
(C5H11)2
or as a Term with a different coefficient e.g.2 C5H11
will not work.The Compound can, however, be in any context such as within a Term and/or Statement.
e.g.
CH3(CH2)8CH3 -> C8H18 + C2H4
Other changes would require a grammar rewrite, so can be handled later if wanted.
Also fixes
atomCount
for Expressions by keeping a separateatomTermCount
andatomBracketCount
to get correctly multiplied by coefficients - rather than multiplying the entire aggregate count.This breaks many of the tests in
Chemistry.test.ts
, but the tests were based on the incorrect previous counting system so this is okay. I will make a card to fix these at some point later.