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

Fix several bugs and minor refactors #5

Merged
merged 2 commits into from
Aug 28, 2017
Merged

Fix several bugs and minor refactors #5

merged 2 commits into from
Aug 28, 2017

Conversation

oscar-stripe
Copy link
Contributor

@oscar-stripe oscar-stripe commented Aug 26, 2017

  • added a more complex example for the tests: Flow which is a very
    simple data-flow style AST (similar to spark, etc...), but hopefully
    complex enough to expose more bugs than the addition example.

  • there was a bug with isRoot, this is fixed

  • there was a bug in find which could result in needless extra Ids added
    to the graph

  • found a proof that the current approach (and maybe all approaches that
    preserve all Ids) cannot maintain the invariant that each N[_] is the
    result of evaluating at most one Id[_]. Which is to say
    evaluate[T](i: Id[T]): N[T]
    is a many to one function.

cc @non @erik-stripe

* added a more complex example for the tests: Flow which is a very
simple data-flow style AST (similar to spark, etc...), but hopefully
complex enough to expose more bugs than the addition example.

* there was a bug with isRoot, this is fixed

* there was a bug in find which could result in needless extra Ids added
to the graph

* found a proof that the current approach (and maybe all approaches that
preserve all Ids) cannot maintain the invariant that each N[_] is the
result of evaluating at most one Id[_]. Which is to say
evaluate[T](i: Id[T]): N[T]
is a many to one function.
@codecov-io
Copy link

codecov-io commented Aug 26, 2017

Codecov Report

Merging #5 into master will increase coverage by 16.87%.
The diff coverage is 95.65%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master       #5       +/-   ##
===========================================
+ Coverage   69.51%   86.38%   +16.87%     
===========================================
  Files          11       12        +1     
  Lines         164      191       +27     
  Branches        6       17       +11     
===========================================
+ Hits          114      165       +51     
+ Misses         50       26       -24
Impacted Files Coverage Δ
...c/main/scala/com/stripe/dagon/DependantGraph.scala 83.33% <100%> (+83.33%) ⬆️
core/src/main/scala/com/stripe/dagon/Graphs.scala 54.54% <100%> (+54.54%) ⬆️
core/src/main/scala/com/stripe/dagon/Id.scala 100% <100%> (ø)
core/src/main/scala/com/stripe/dagon/Rule.scala 75% <83.33%> (+75%) ⬆️
...rc/main/scala/com/stripe/dagon/ExpressionDag.scala 94% <96.49%> (+4.84%) ⬆️
core/src/main/scala/com/stripe/dagon/HMap.scala 77.77% <0%> (+5.55%) ⬆️

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 8bdb8cf...893cc9c. Read the comment docs.

@non
Copy link
Collaborator

non commented Aug 28, 2017

👍

@non non merged commit 97c2f2e into master Aug 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants