Skip to content

HLists for Slick#208

Merged
szeiger merged 2 commits intomasterfrom
tmp/shape-improvements
Nov 4, 2013
Merged

HLists for Slick#208
szeiger merged 2 commits intomasterfrom
tmp/shape-improvements

Conversation

@szeiger
Copy link
Copy Markdown
Member

@szeiger szeiger commented Sep 11, 2013

Add a custom HList implementation with a corresponding Shape, and simplify the handling of custom Shapes a bit.

Review by @cvogt

@nafg
Copy link
Copy Markdown
Member

nafg commented Sep 18, 2013

Why not use shapeless?
On Sep 11, 2013 9:33 AM, "Stefan Zeiger" notifications@github.com wrote:

Add a custom HList implementation with a corresponding Shape, and simplify
the handling of custom Shapes a bit.

Review by @cvogt https://github.com/cvogt

You can merge this Pull Request by running

git pull https://github.com/slick/slick tmp/shape-improvements

Or view, comment on, or merge it at:

#208
Commit Summary

  • Simplify custom Shape definitions
  • Add HList and Nat implementations with Shapes for the HList.
  • Simplify MapperTest.testCustomShape.

File Changes

  • M
    slick-testkit/src/main/scala/com/typesafe/slick/testkit/tests/MapperTest.scalahttps://github.com/HLists for Slick #208/files#diff-0(121)
  • A
    slick-testkit/src/test/scala/scala/slick/test/collection/heterogenous/HListTest.scalahttps://github.com/HLists for Slick #208/files#diff-1(41)
  • A
    slick-testkit/src/test/scala/scala/slick/test/collection/heterogenous/NatTest.scalahttps://github.com/HLists for Slick #208/files#diff-2(49)
  • A src/main/scala/scala/slick/collection/heterogenous/HList.scalahttps://github.com/HLists for Slick #208/files#diff-3(183)
  • A src/main/scala/scala/slick/collection/heterogenous/Nat.scalahttps://github.com/HLists for Slick #208/files#diff-4(182)
  • A
    src/main/scala/scala/slick/collection/heterogenous/TypedFunction.scalahttps://github.com/HLists for Slick #208/files#diff-5(8)
  • A src/main/scala/scala/slick/collection/heterogenous/syntax.scalahttps://github.com/HLists for Slick #208/files#diff-6(20)
  • M src/main/scala/scala/slick/lifted/Aliases.scalahttps://github.com/HLists for Slick #208/files#diff-7(3)
  • M src/main/scala/scala/slick/lifted/Shape.scalahttps://github.com/HLists for Slick #208/files#diff-8(31)

Patch Links:

@szeiger
Copy link
Copy Markdown
Member Author

szeiger commented Sep 18, 2013

I don't want to have a dependency on Shapeless for this feature, so I added a small standalone implementation that covers the typical use cases in Slick. Support for Shapeless HLists should be trivial to implement (5 lines of code?). We could do this with a separate module once we have automated builds.

@szeiger
Copy link
Copy Markdown
Member Author

szeiger commented Sep 20, 2013

Rebased

@nafg
Copy link
Copy Markdown
Member

nafg commented Sep 30, 2013

At least maybe it should be a separate module/artifact in the codebase that could be published with slick and that slick would depend upon?

@szeiger
Copy link
Copy Markdown
Member Author

szeiger commented Oct 1, 2013

I don't see the need for putting HLists into a separate module. The way they are written, they require Slick (because the Shapes are defined in the companion objects) and it's very little extra code. HList support for other libraries like Shapeless would have to go into separate modules which depend on Slick and the 3rd-party library.

@ijuma
Copy link
Copy Markdown
Contributor

ijuma commented Oct 2, 2013

I think @nafg meant that it may be useful to have a slick-shapeless module that would use HLists from shapeless for those that don't mind the dependency. I think that can be added later. For now, having a simple HList in Slick seems like a good step forward.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The name Fold is misleading. This is not http://en.wikipedia.org/wiki/Fold_%28higher-order_function%29 . It is closer to something like http://reference.wolfram.com/mathematica/ref/Nest.html . It applies F (N+1) times to Z. Certainly needs a doc comment.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

But applying f (n+1) times to z is the catamorphism or fold for Nat (which essentially turns the Peano structure into Church Numerals)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You are right. Added a doc comment to #231 . This should be enough.

@szeiger
Copy link
Copy Markdown
Member Author

szeiger commented Nov 1, 2013

@cvogt WDYT?

@szeiger szeiger mentioned this pull request Nov 1, 2013
@cvogt
Copy link
Copy Markdown
Member

cvogt commented Nov 1, 2013

LGTM (and thx for the separate commit, made it easy to review!)

- Simplify MapperTest.testCustomShape. No need to introduce
  another HList here, now that we have a real one in Slick.

Tests in HListTest, NatTest and MapperTest.testHList.
@szeiger
Copy link
Copy Markdown
Member Author

szeiger commented Nov 4, 2013

Rebased, reordered and squashed

szeiger added a commit that referenced this pull request Nov 4, 2013
@szeiger szeiger merged commit fa5536f into master Nov 4, 2013
@szeiger szeiger deleted the tmp/shape-improvements branch November 4, 2013 11:49
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.

4 participants