Refactor hybrid text parsing to account for arbitrarily-nested ANDs and ORs

[Purplegaze]

Proposed Changes

Currently, hybrid text nodes are only equipped to handle a single AND of ORs, which makes the 2nd year calculus hybrid node ((MAT235,MAT236)/MAT237/MAT257) not work properly. This PR fixes this bug by making hybrid text parsing recursive and changing the code that updates the state and relatives of hybrid nodes to work separately from regular nodes and account for arbitarily-nested course prerequisite strings.

...

Screenshots of your changes (if applicable)

Type of Change

(Write an X or a brief description next to the type or types that best describe your changes.)

Type	Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)
✨ New feature (non-breaking change that adds functionality)
🐛 Bug fix (non-breaking change that fixes an issue)	X
🎨 User interface change (change to user interface; provide screenshots)
♻️ Refactoring (internal change to codebase, without changing functionality)
🚦 Test update (change that only adds or modifies tests)
📦 Dependency update (change that updates a dependency)
🔧 Internal (change that only affects developers or continuous integration)

Checklist

(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the [ ] into a [x] in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)

Before opening your pull request:

• I have performed a self-review of my changes.
  • Check that all changed files included in this pull request are intentional changes.
  • Check that all changes are relevant to the purpose of this pull request, as described above.
• I have added tests for my changes, if applicable.
  • This is required for all bug fixes and new features.
• I have updated the project documentation, if applicable.
  • This is required for new features.
• If this is my first contribution, I have added myself to the list of contributors.
• I have updated the project Changelog (this is required for all changes).

After opening your pull request:

• I have verified that the CircleCI checks have passed.
• I have requested a review from a project maintainer.

Questions and Comments

See below.

[Purplegaze]

Hello, here's my draft PR for this issue.

My current solution is to split updateNode to handle regular and hybrid nodes separately, by having a new function arePrereqsSatisfiedHybrid that determines the state of a hybrid node between active and inactive with the recursive parsing framework you described. Then, every course contained within a hybrid node is added (flattened into one list) to the parent-child relationships so that states are properly checked when they need to be.

The existing code that updates nodes did not ever set hybrid nodes to anything that isn't active or inactive, so I believe the updateNode logic is unchanged here. However, I noticed takeable can be a state that was applied to hybrid nodes when they are first initialized (line 191 of Graph.js), and the Stats graph also has takeable hybrid nodes right now, though they appear not to function properly. I'm wondering if I need to update this or pursue a different approach to get the takeable state to function in hybrid nodes.

Another approach would be to change the functionality of parent-child relationships as well, and add the full nested layers of nodes to them, since currently parent-child relationships only work on "ANDs of ORs" as well, though that might be an overly complicated thing to implement if the only thing that's necessary is to test whether a hybrid node is active.

Also, with recursive parsing, it becomes more difficult to have parentheses be able to open up in the middle of a course name, such as in the string MAT(135,136)/137/157 as was in one of the tests, which I think would throw this function in an infinite loop (at least until the maximum recursion depth is hit). It is probably possible by somehow combining recursive logic with the iterative parsing that existed before, but since no hybrid nodes appear to use this level of shorthand in the graphs, I've left it out right now. Basic shorthand that doesn't go in and out of parenthesis layers (like MAT137/157) still works. Let me know if you'd like me to still implement the more complex type of shorthand.

[coveralls]

Coverage Report for CI Build 0

Coverage increased (+0.4%) to 58.567%

Details

• Coverage increased (+0.4%) from the base build.
• Patch coverage: 5 uncovered changes across 1 file (88 of 93 lines covered, 94.62%).
• No coverage regressions found.

Uncovered Changes

File	Changed	Covered	%
js/components/graph/Graph.js	35	30	85.71%
Total (2 files)	93	88	94.62%

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	4273
Covered Lines:	2556
Line Coverage:	59.82%
Relevant Branches:	1003
Covered Branches:	534
Branch Coverage:	53.24%
Branches in Coverage %:	Yes
Coverage Strength:	155.46 hits per line

---

💛 - Coveralls

[david-yz-liu]

Hi @Purplegaze, thanks for the detailed update. Overall I think this is the right approach.

• The split to a new arePrereqsSatisfiedHybrid function is good
• It's fine for hybrid nodes to only have active/inactive states, so let's not worry about "takeable" for these nodes.
• No need to worry about parentheses within a course name, e.g. "MAT(135,136)".