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 type safe projection #271

Closed
wants to merge 4 commits into from
Closed

Add type safe projection #271

wants to merge 4 commits into from

Conversation

To-om
Copy link
Contributor

@To-om To-om commented Feb 24, 2019

This PR adds type safe version of project. This uses a ProjectionBuilder class to build HList type. I choose to not expose projection label (only order of projections counts):

graph
        .V()
        .out("created")
        .project(_
          .apply(_.value(Key[String]("name")))
          .and(_.in("created").count()))
        .head()
// returns "lop" :: 3 :: HNil with correct type

@mpollmeier
Copy link
Owner

This is great, thank you! I'd prefer to not expose HList to the user, instead we should transform it to tuples, which is possible (used e.g. in select). What do you think?

Also note that #241 by @jeremysears is going in a similar direction - I tried to chip in but didn't manage to finish it.

@To-om
Copy link
Contributor Author

To-om commented Feb 26, 2019

I agree with you. HList complexity should not be exposed to user.

@mpollmeier
Copy link
Owner

I just took the liberty to refactor the testcase, let me know if you like it: a141bb4

For the 'simple' case of having only one By modulation, the result type is always derived as a Tuple1, which is clunky to work with - have a look at the test I added. I'm guessing shapeless can map that to the wrapped type, but no idea how clunky that becomes... thoughts?

This is fun!

@To-om
Copy link
Contributor Author

To-om commented Feb 27, 2019

I don't think that Tuple1 is problematic as the use of project for only one projection is non-sense (you can simply remove the project step).
If you really want to unwrap Tuple1, it is possible to add a new layer of implicit that transforms Tuple1[T] into T and other types by themselves. IMHO it adds more useless complexity.

@mpollmeier
Copy link
Owner

🤦‍♂️ of course, you're right.

I dropped that nonsense test, moved ProjectionBuilder to it's own file: #273
I'll merged to master once CI is green, then it will release automagically. Thank you!

@mpollmeier mpollmeier closed this Feb 27, 2019
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.

None yet

2 participants