#415 Move ScopeDepth from ParseState to ParseGraph. #422
Conversation
Kept it to ParseState for now, so we can compare the scopedepth in both objects during development. ScopeDepth should be removed from ParseState later.
…e message. Decided to redefine the method so we do not have to add the String.format part for every call. Also, this way the change in the class is minimized.
Also converted the class to have a setup method which sets the static fields. This way I can use the variables when creating the stream of arguments for the parameterized test.
Use the scopeDepth of the ParseGraph within the ParseState instead. Since the ParseState does not contain a scopeDepth parameter anymore, for the ParseValueCacheTest, we need to build a ParseGraph with a scopeDepth of 2 instead.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #422 +/- ##
===========================================
Coverage 100.00% 100.00%
- Complexity 1150 1153 +3
===========================================
Files 98 98
Lines 1537 1543 +6
Branches 159 164 +5
===========================================
+ Hits 1537 1543 +6 ☔ View full report in Codecov by Sentry. |
The check within addBranch is too late. We can already detect the illegal state when we want to close a branch such that we parse graph gets into non-branched state. If the scopeDepth is then non-zero, which should not be possible, then something was wrong before and we should exit. This is a situation we can also test. Note that it will only occur if the user creates a custom token that changes its scope delimiter setting while processing. This is not the case in within Metal, but found it valuable to add it anyway.
…edValues. It is tested indirectly by calling getAllValues with scope parameters.
This previously was the case, but because of static imports, it was not aligned anymore.
| @@ -437,4 +436,11 @@ public void basicNoHashCollisions(final Object object, final Object same, final | |||
| } | |||
| } | |||
|
|
|||
| private static void assertEquals(final Object o, final Object object) { | |||
There was a problem hiding this comment.
Decided to override the assertEquals methods, so we get more meaningful error messages when a test fails. You will now see which objects causes a test to fail, which previously could only be discovered by debugging the test and then print the objects and do a manual comparison, which takes much more time.
| private final ParseValue g; | ||
| private final ParseValue h; | ||
|
|
||
| public ParseGraphTest() { |
There was a problem hiding this comment.
Instead of using a constructor, decided to use a @BeforeAll setup method instead. This way, I could use the fields within the arguments of the ParameterizedTest here.
| } | ||
| return new ParseGraph(head, tail, definition, false); | ||
| return new ParseGraph(head, tail, definition); |
There was a problem hiding this comment.
Nice change, to call the other constructor here (to make the false implicit).
| @@ -140,6 +150,7 @@ public boolean equals(final Object obj) { | |||
| && Objects.equals(head, ((ParseGraph)obj).head) | |||
| && Objects.equals(tail, ((ParseGraph)obj).tail) | |||
| && Objects.equals(branched, ((ParseGraph)obj).branched) | |||
| && Objects.equals(scopeDepth, ((ParseGraph)obj).scopeDepth) | |||
There was a problem hiding this comment.
The scopeDepth is essentially derivable right? Maybe it should be considered cached then as well just like size?
There was a problem hiding this comment.
With normal usage, it should be derivable. However, it is possible to create a token that alternates its scope delimiter (see this test token). This makes it possible to have two identitcal ParseGraphs with different scope depths. This is only theoretical and I do not know why you would create a token like that, but for robustness it is better to keep this here. Other minor reason to keep it here is so we can test equality for the ParseGraph with the AutoEqualityTest.
|
|
||
| public ParseState(final ParseGraph order, final ParseValueCache cache, final Source source, final BigInteger offset, final ImmutableList<ImmutablePair<Token, BigInteger>> iterations, final ImmutableList<ParseReference> references, final int scopeDepth) { |
There was a problem hiding this comment.
Cleans up ParseState a lot to move scopeDepth to ParseGraph!
| @@ -236,4 +246,130 @@ public void testCurrent() { | |||
| assertFalse(EMPTY.addBranch(NONE).current().isPresent()); | |||
| } | |||
|
|
|||
| public static Stream<Arguments> scopeDepthTest() { | |||
There was a problem hiding this comment.
Wow, amazing work on these extensive tests!
…yTest. The ParseGraph contain multiple private constructors and they are used as copy constructors. It contains the cached value for scopeDepth that matches the depth of the head. It is not possible to have two ParseGraphs with the same head and tail, but with a different ScopeDepth, but this is a case that will be testing using AutoEqualityTest. We need to write our own equality test for parsegraph.
…a/metal into #415_scopeDepth_to_parseGraph
Since it is removed from the AutoEqualityTest, included the test to ParseGraphTest instead.
Although it is not a useful usecase, it is possible to create to identical parse graphs with different scopedepth, by creating a token that alternates its scope delimiter setting. That means that the scope depth stored within the parse graph is actually not a cached value and must therefor be included within the equals method. In that case, we can test the equality of the ParseGraph by the AutoEqualityTest.
Not actually part of this branch, but it was a remark when updating the maven surefire plugin in a previous issue, but was missed.
See also issue #415.
When de scopeDepth is stored within the ParseState, then calling
withOrderwith a different parseGraph, should actually reset the scopeDepth. Since it is dependendant on the parseGraph it makes more sense to store it there.