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

default out should be Stream<X>, not Optional<Stream<X>> #40

Merged
merged 17 commits into from
Mar 21, 2015

Conversation

eparejatobes
Copy link
Member

What the title says. Everyone's going to use Stream taking empy into account, and a NEStream type in Java is not a viable alternative. This involves an API change, so I 'd like to do this ASAP.

cc @laughedelic

@eparejatobes
Copy link
Member Author

also, now that we're at it: out -> outE

@laughedelic
Copy link
Member

👍

@eparejatobes
Copy link
Member Author

I started with this and now outManyOptional is even more confusing than before. I will write a first version where there's no outManyOptional; just use out in that case.

@eparejatobes
Copy link
Member Author

@pablopareja @laughedelic a summary of how things are here

  • There's a suffix for all in/out methods now: xxxE for edges, xxxV for vertices
  • Optional is only used in the (in/out)OneOptional methods, which return Optional<X>
  • The default case is Stream<X>, which means that (in/out)ManyOptional does not make any sense now; I removed it.
  • What about (in/out)Many? this is arguable; it could be used to signal that you're going to return a non-empty Stream. I don't know.

thoughts?

@laughedelic
Copy link
Member

I agree with (in/out)Many and I would also replace (in/out)OneOptional with just (in/out)Optional. Are there other things, like exactly one or at least one?

@eparejatobes
Copy link
Member Author

yes, there's (in/out)One

@laughedelic
Copy link
Member

ok, so I think it would be good to have

  • (in/out)One
  • (in/out)Optional
  • (in/out)Many

@eparejatobes
Copy link
Member Author

+1 from me

@eparejatobes
Copy link
Member Author

@laughedelic @pablopareja please review. I'm happy with the current state. The names for arities are far from perfect, but I cannot think of anything better right now. Suggestions more than welcome

@pablopareja
Copy link
Member

I agree with @laughedelic, I would keep

  • (in/out)One
  • (in/out)Optional
  • (in/out)Many

and then I would add a new naming for the ones returning an empty/non-empty Stream which would be

  • (in/out)Any

@eparejatobes
Copy link
Member Author

@pablopareja fine. I don't get the last part about inAny though

@eparejatobes
Copy link
Member Author

but that's just what a Stream is, right?

@pablopareja
Copy link
Member

yes but since the arity of the results is expressed in the function name for the rest of the cases I think it is quite straightforward to do the same in the case of the Stream (otherwise it doesn't make much sense to ever include this information)

@eparejatobes
Copy link
Member Author

In general the return type is a Stream; if you know more about it you can then say that it is

  • a Stream with exactly one element -> One
  • a Stream with at most one element -> Optional
  • a Stream with at least one element -> Many

you always have in/out with no qualifiers, which returns a Stream. What's the point of having something with a different name doing exactly the same?

@pablopareja
Copy link
Member

I don't know, at least for me it's much more intuitive to have the qualifier Any in the case where you can get any quantity of elements without having to think about the implication that the returning type is a Stream and thus any amount of elements could be returned...
I see it as a more understandable method naming but that's just my view...

@eparejatobes
Copy link
Member Author

Another, more verbose, option is to use as suffixes

  • One
  • AtMostOne
  • AtLeastOne
  • (empty) which means the generic case where you don't know anything

This would fit better with a somewhat more intuitive naming scheme for arities:

  • From(qualifier)To(qualifier)

where we could leave any of them empty if there's nothing to say about it. This would yield

public enum arity {

  fromOneToOne,
  fromOneToAtMostOne,
  fromOneToAtLeastOne,
  fromOne, 

  fromAtMostOneToOne,
  fromAtMostOneToAtMostOne,
  fromAtMostOneToAtLeastOne,
  fromAtMostOne, 

  fromAtLeastOneToOne,
  fromAtLeastOneToAtMostOne,
  fromAtLeastOneToAtLeastOne,
  fromAtLeastOne,

  toOne,
  toAtMostOne,
  toAtLeastOne,
  // whatever
  any;
}

WDYT?

@pablopareja
Copy link
Member

Sounds good to me but, why don't adding Any when there's nothing to say about it? 😃

fromAnyToOne ?

🐙

@laughedelic
Copy link
Member

I'm fine with Any, but I would drop the from prefix

@eparejatobes
Copy link
Member Author

public enum arity {

  oneToOne,
  oneToAtMostOne,
  oneToAtLeastOne,
  oneToAny, 

  atMostOneToOne,
  atMostOneToAtMostOne,
  atMostOneToAtLeastOne,
  AtMostOneToAny, 

  atLeastOneToOne,
  atLeastOneToAtMostOne,
  atLeastOneToAtLeastOne,
  atLeastOneToAny,

  anyToOne,
  anyToAtMostOne,
  anyToAtLeastOne,
  // whatever
  anyToAny;
}

@laughedelic
Copy link
Member

👍

1 similar comment
@pablopareja
Copy link
Member

👍

@eparejatobes
Copy link
Member Author

I think we should keep the in/out suffixes the same though; inAtMostOneV(rel) sounds weird, like give me only those vertices which have at most one rel in.

@eparejatobes
Copy link
Member Author

@laughedelic @pablopareja please review

@laughedelic
Copy link
Member

I didn't understand you last comment (#40 (comment)). Otherwise it seems to be ok.

@laughedelic
Copy link
Member

So what do you suggest? to leave it inOptionalV or to change to the inAtMostOneV?

@eparejatobes
Copy link
Member Author

@pablopareja
Copy link
Member

I'm a bit lost too...
So this:

public enum arity {

  oneToOne,
  oneToAtMostOne,
  oneToAtLeastOne,
  oneToAny, 

  atMostOneToOne,
  atMostOneToAtMostOne,
  atMostOneToAtLeastOne,
  AtMostOneToAny, 

  atLeastOneToOne,
  atLeastOneToAtMostOne,
  atLeastOneToAtLeastOne,
  atLeastOneToAny,

  anyToOne,
  anyToAtMostOne,
  anyToAtLeastOne,
  // whatever
  anyToAny;
}

is not valid anymore?

@eparejatobes
Copy link
Member Author

yes, just look at the code. Arities are defined in TypedEdge.

@pablopareja
Copy link
Member

👍

@eparejatobes
Copy link
Member Author

I'm merging this, and finishing planning what we want to be in 0.5

@eparejatobes eparejatobes merged commit 2ff82ef into master Mar 21, 2015
@eparejatobes eparejatobes deleted the feature/stream/only/api branch March 21, 2015 19:35
@eparejatobes eparejatobes mentioned this pull request Mar 21, 2015
eparejatobes added a commit that referenced this pull request Sep 3, 2015
Several breaking changes here, but mostly about names.

1. We dropped `Optional<Stream<X>>` in favor of just `Stream<X>`. An empty stream signals the equivalent of none before. See #40
2. We are using `E` as a _suffix_ for methods related with edges like `outE`, `inE`.
3. The names for arities have changed. See #41 and #40. They are defined in `TypedEdge.java`.

Apart from that, documentation should display better on github.com. The build plugin was also updated.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants