Skip to content
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

Add SingleValueExpression to type ValueExpressions that always return a single value #283

Merged
merged 8 commits into from
Feb 16, 2019

Conversation

jvdb
Copy link
Contributor

@jvdb jvdb commented Feb 11, 2019

This PR merges to the branch of #282. So please review that first.

Introduces SingleValueExpression as proposed in #70.

Instead of the ImmutableList<Value> that is returned by ValueExpression.eval(...), SingleValueExpression has a evalSingle(...) method with the same arguments that returns an Optional<Value>. SingleValueExpression extends ValueExpression, so that it can be used wherever a ValueExpression is required.

The default implementation of the eval(...) method on SingleValueExpression converts an empty result of evalSingle(...) to an empty list, otherwise the returned value is stored as single value in the returned list.

Converted to SingleValueExpression are:

  1. Token arguments:
    1. the size argument of Def
    2. the size argument of Nod (essentially just a proxy for Def)
    3. the n argument of RepN
  2. ValueExpression implementations:
    1. CurrentOffset
    2. CurrentIteration
    3. Const
    4. First
    5. Last
    6. Count
    7. Self
  3. ValueExpression arguments:
    1. the count argument of Expand
    2. the initial argument of Fold

Tasks:

  • The Fold implementations themselves should also be converted, not just (some of) the argument(s).
  • Resolve static analysis tools results.

The unaddressed tasks that was in the list concerning refactoring the type of Fold's reducer argument now has its own issue: #284.

This PR resolves #70.

@codecov
Copy link

codecov bot commented Feb 11, 2019

Codecov Report

Merging #283 into value-interface will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@               Coverage Diff                @@
##             value-interface   #283   +/-   ##
================================================
  Coverage                100%   100%           
- Complexity              1043   1046    +3     
================================================
  Files                     93     94    +1     
  Lines                   1406   1399    -7     
  Branches                 154    150    -4     
================================================
- Hits                    1406   1399    -7
Impacted Files Coverage Δ Complexity Δ
.../parsingdata/metal/expression/value/CoreValue.java 100% <ø> (ø) 18 <0> (ø) ⬇️
...o/parsingdata/metal/expression/value/FoldLeft.java 100% <ø> (ø) 3 <0> (ø) ⬇️
.../parsingdata/metal/expression/value/FoldRight.java 100% <ø> (ø) 3 <0> (ø) ⬇️
...singdata/metal/expression/value/reference/Nth.java 100% <ø> (ø) 19 <0> (ø) ⬇️
...l/expression/value/reference/CurrentIteration.java 100% <ø> (ø) 15 <0> (-1) ⬇️
...rc/main/java/io/parsingdata/metal/format/JPEG.java 100% <100%> (ø) 1 <0> (ø) ⬇️
...ingdata/metal/expression/value/reference/Last.java 100% <100%> (ø) 8 <0> (ø) ⬇️
...ingdata/metal/expression/value/reference/Self.java 100% <100%> (ø) 5 <2> (ø) ⬇️
...src/main/java/io/parsingdata/metal/token/RepN.java 100% <100%> (ø) 11 <5> (ø) ⬇️
.../src/main/java/io/parsingdata/metal/Shorthand.java 100% <100%> (ø) 131 <33> (ø) ⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87e0260...60fedd2. Read the comment docs.

Copy link
Contributor

@rdvdijk rdvdijk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

The only thing that looks like something that could be improved is that we now need to add last() around a few expressions.

Another thing is that last() will not throw an exception, where the old ValueExpressions would explicitly check for lists of length 1, and throw an exception in the other cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants