#381 Move Scope ValueExpressions as parameter of Ref. #411
Conversation
The scope only makes sense in combination with ref. It has no function for other value expressions. When it is included within the ref, we can also make it more efficient, because we can check the scopeDepth while traversing the parseGraph for the values in a single run. Co-authored-by: jvdb <[email protected]>
The previous commit contains API breaking changes.
These seem to be redundant, since the limit can be specified using a variant of `ref` instead.
Also included test for refs with limit 0, which return an empty list immediately in the new getAllScopedValues. If limit is 0, not need to go find the correct scope and we can return immediately.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #411 +/- ##
===========================================
Coverage 100.00% 100.00%
+ Complexity 1153 1150 -3
===========================================
Files 99 98 -1
Lines 1536 1537 +1
Branches 157 159 +2
===========================================
+ Hits 1536 1537 +1 ☔ View full report in Codecov by Sentry. |
The static import already existed apparently...
| @@ -349,7 +348,11 @@ private Shorthand() {} | |||
| public static BinaryValueExpression mapRight(final BiFunction<ValueExpression, ValueExpression, BinaryValueExpression> func, final SingleValueExpression leftExpand, final ValueExpression right) { return func.apply(exp(leftExpand, count(right)), right); } | |||
|
|
|||
| /** @see Bytes */ public static ValueExpression bytes(final ValueExpression operand) { return new Bytes(operand); } | |||
| /** @see Scope */ public static ValueExpression scope(final ValueExpression scopedValueExpression, final SingleValueExpression scopeSize) { return new Scope(scopedValueExpression, scopeSize); } | |||
|
|
|||
| /** @see Ref */ public static NameRef scope(final NameRef operand) { return scope(operand, con(0)); } | |||
There was a problem hiding this comment.
Do we think these shorthands (with the default scope of 0) could be useful?
There was a problem hiding this comment.
Maybe, but I'm not sure about the name, maybe scopedRef? But maybe all Refs that have the scopeSize-argument should be called that? I haven't given it much thought yet, but it might be worth considering.
There was a problem hiding this comment.
What would be a typical use case for a Ref with scope of 0?
There was a problem hiding this comment.
That means the most local scope that is possible, e.g. only values in the current seq.
I think it is the scope level that will be used most, hence the shorthand for it.
There was a problem hiding this comment.
Maybe, but I'm not sure about the name, maybe
scopedRef? But maybe allRefs that have thescopeSize-argument should be called that? I haven't given it much thought yet, but it might be worth considering.
If we would call it scopedRef, then usage would look like scopedRef(ref("name")). I like to keep it scope shorthand, especially since it is now always a ref, so including Ref is redundant to me.
|
|
||
| public ParseState(final ParseGraph order, final ParseValueCache cache, final Source source, final BigInteger offset, final ImmutableList<ImmutablePair<Token, BigInteger>> iterations, final ImmutableList<ParseReference> references) { | ||
| 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.
A constructor with seven arguments, oof 😅
I don't think we should address this now, but we should discuss this when/if we refactor the ParseGraph implementation.
There was a problem hiding this comment.
I just created #415 , where we are going to move the scopeDepth to the ParseGraph instead. This way, the withOrder method can be used correctly and the ParseState will not hold an incorrect scopeDepth. I developed it in our fork for now, but will create a separate PR here soon.
The Scope value expression makes no sense without a Ref.
The Scope class is removed and the scopeLevel is moved to a parameter for Ref instead.
Changed tests accordingly.
Since the scope level and the limit level are now both available within a Ref, we only have to traverse the ParseGraph once and keep track of both parameters when collecting values. This may have a slight performance gain.
Also note, that previously the ParseValueCache was never used in combination with Scope class. Now, we can check the value of the scopeLevel to determine if we can use the cache or not.