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

Classes all the way down #58

Closed
wants to merge 21 commits into from

Conversation

laughedelic
Copy link
Member

No description provided.

…ce/target methods won't work (now wee need to bind all parameters 👎)
@laughedelic
Copy link
Member Author

This is more or less ready as a proof-of-the-concept. source/target, outE/outV, addVertex/addEdge are implemented and seem to work. See the test code for examples of usage.

@laughedelic
Copy link
Member Author

Some summary of what we have with this approach. Actually, this version is not less verbose than what we have from #57:

  • we get rid of having V,VT pairs, because V is now an inner class Vertex of VT
  • but we cannot write VT.Vertex anywhere! 😩 (unless VT is a concrete class, like User)
  • if VT extends VertexType<VT, G,RV,RE>, everywhere we want to refer to its Vertex inner class, we have to write VertexType<VT, G,RV,RE>.Vertex 👎
  • note, that with this restriction, we have to bind all VT's parameters if we are going to return a value that uses them, because we cannot return VertexType<VT, ?,RV,RE>.Vertex
  • this basically means that we have to carry everywhere edge's source/target graphs and bind them explicitly whenever source and target are involved

This approach has its advantages and disadvantages:

  • On one hand, if we promote the feature of having edges between graphs, then it's normal to mention those source/target graphs. Plus making everything being classes simplifies things and let us be more concise.
  • On the other hand, this VertexType<VT, G,RV,RE>.Vertex everywhere makes this approach quite verbose. Although it's only in the library inner code. Once you define schema, you don't need to worry about it. You will get your TwitterSchema<RV,RE>.Tweet.Vertex.

@laughedelic
Copy link
Member Author

  • Particular graph schema has to be a final class
  • Every element of the schema has a reference to the containing graph (on the type level)
  • Use precise types for inner classes instances (VT.Vertex instead of VertexType<VT,G,RV,RE>.Vertex), even if the "unchecked call" warning has to be suppressed (in .fromRaw()
  • move all the API methods to the entity classes, make TypedGraph class (move here things from GraphSchema)
  • same everything for indexes (class with all params, refined in the graph class)
  • add arity-specific methods to the UntypedGraph with default impl-s
  • add LinkGraph with explicit references to the two graph it's connecting


return vertexType.fromRaw(
return vertexType.new Vertex(
Copy link
Member Author

Choose a reason for hiding this comment

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

@eparejatobes take a look, please. If I use .fromRaw here, I get

com.bio4j.angulillos.VertexType.Vertex cannot be converted to 
com.bio4j.angulillos.VertexType.Vertex

😆

Copy link
Member

Choose a reason for hiding this comment

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

I guess you need VT extends G.VertexType<...

Copy link
Member Author

Choose a reason for hiding this comment

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

At this moment there is no G.VertexType yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, there is something funny happening. I've tried changing that many times and sometimes I cannot get rid of these errors, but then if I clean (well) and compile, it compiles fine. I just tried to make a little snippet of code with the same pattern and I couldn't reproduce this compilation error, so I think that it's some incremental compilation artefacts..

@@ -14,8 +14,9 @@

public TwitterSchema<RV,RE>.User.Vertex addUser(String name, Integer age) {

return g.addVertex(g.user)
.set(g.user.name, name)
TwitterSchema<RV,RE>.User.Vertex u = g.user.addVertex();
Copy link
Member Author

Choose a reason for hiding this comment

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

@eparejatobes
In the end I think we still need to write VertexType<VT, ...>.Vertex instead of just VT.Vertex in the abstract code. because on this line I get

[warn] /Users/laughedelic/dev/bio4j/angulillos/src/test/java/com/bio4j/angulillos/TwitterGraphTestSuite.java:17: unchecked conversion
[warn]   required: com.bio4j.angulillos.VertexType<com.bio4j.angulillos.TwitterSchema<RV,RE>.User,com.bio4j.angulillos.TwitterSchema<RV,RE>,RV,RE>.Vertex
[warn]   found:    com.bio4j.angulillos.VertexType.Vertex
[warn]     TwitterSchema<RV,RE>.User.Vertex u = g.user.addVertex();

and it means that the VT.Vertex addVertex() signature is interpreted as VertexType.Vertex addVertex(), because at that moment everything the compiler know is that VT <: VertexType (without params).

Copy link
Member

Choose a reason for hiding this comment

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

yes, but then how are we gettting the right type? through calling the right constructor once an schema has been implemented?

Copy link
Member Author

Choose a reason for hiding this comment

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

The thing is that VertexType<VT, ...>.Vertex is the right type, once VT is fixed. Look at the "expected" type in the warning message. I can write

// VT.Vertex
TwitterSchema<RV,RE>.User.Vertex u1 = null; // just to have something of this type

// VertexType<VT, G,RV,RE>.Vertex
VertexType<
  TwitterSchema<RV,RE>.User, 
  TwitterSchema<RV,RE>, 
  RV,RE
>.Vertex u2 = u1;

and another way round. And there won't be any warnings about unchecked conversions.

@laughedelic
Copy link
Member Author

I was looking for some information about self-reference pattern in Java and I found this SO answer plus some other places, where people write that the bound on the self-type parameter

class Foo<F extends Foo<F>>

is an illusion of safety and does not enforce F to be self-reference. I still can write

  public final class Tweet extends VertexType<User> {
    @Override public User self() { return user; }
    ...
  }

P.S. this just means that we cannot refer to the self-type in the abstract code when using something specific about its instance. So I'll just write VertexType<...>.Vertex where needed. It does not influence the way this is used (e.g. things like TwitterSchema<RV,RE>.User.Vertex will be fine and correct). I already had this before 758be84.

@eparejatobes
Copy link
Member

Yup, http://stackoverflow.com/a/14755312/614394

It's not exactly the same with mutually recursive parameters, though.

@eparejatobes
Copy link
Member

About the warnings: my somewhat cryptic (writing from my phone) comment #58 (comment) tried to imply that for that you'd need to make everything inner classes. If so, it should probably work.

@laughedelic
Copy link
Member Author

  • make self() protected
  • use final methods for fields everywhere
  • split arity in two, use in property, add related methods
  • "revert" 758be84 (writing super-verbose VertexType<VT, ...>.Vertex everywhere)

@laughedelic
Copy link
Member Author

Good news @eparejatobes!!! We don't have to change anything related to the inner-classes.

I was about to fall asleep when I suddenly realised that the problem in the above mentioned example is in the misuse of the types there. First, there is

G extends TwitterSchema<RV,RE>

then

protected G g;

and then I was trying to create a user-vertex: g.user.addVertex() and of course it didn't match the type TwitterSchema<RV,RE>.User.Vertex, because its type is G.User.Vertex!

// This warns about unchecked conversion:
TwitterSchema<RV,RE>.User.Vertex tu = g.user.addVertex();

// This is correct:
G.User.Vertex gu = g.user.addVertex();

// This won't compile:
VertexType<G.User, G,RV,RE>.Vertex gu2 = g.user.addVertex();
// Neither this:
G.VertexType<G.User>.Vertex gu3 = g.user.addVertex();

I think that the warning message is just very confusing, showing as the "found" type com.bio4j.angulillos.VertexType.Vertex, which is being read literally just isn't true.

… to define a propety/vertex type with an existing label, on the graph instantiation you will get an exception)
@eparejatobes
Copy link
Member

@laughedelic but then you'd need to add suppress warnings to all methods, basically because we cannot know that G has the same RV,RE.

@laughedelic laughedelic mentioned this pull request Mar 30, 2016
6 tasks
@laughedelic
Copy link
Member Author

Replaced by #62

@eparejatobes eparejatobes deleted the refactoring/everything-is-a-class branch October 12, 2016 07:51
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.

2 participants