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

Arguments for ActiveRel.creates_unique, new Association `unique` options #1038

Merged
merged 7 commits into from Nov 12, 2015

Conversation

Projects
None yet
2 participants
@subvertallchris
Member

subvertallchris commented Nov 6, 2015

This is RE: #1035. It forces ActiveRel's creates_unique and Association's unique option to take an argument.

  • :none means that all properties are ignored where CREATE UNIQUE is concerned, properties will be set after the rel is created, like our current expectations
  • :all means that any property's change will result in a new rel
  • {on: [keys]} defines which properties it uses for the match

Called with no arguments or true, it will default to :none.

(This has been edited to reflect the end results. Comments below reflect some earlier content here.)

@subvertallchris subvertallchris changed the title from Creates unique with props to Arguments for ActiveRel.creates_unique Nov 6, 2015

@subvertallchris subvertallchris changed the title from Arguments for ActiveRel.creates_unique to Arguments for ActiveRel.creates_unique, new Association `unique` options Nov 7, 2015

@subvertallchris

This comment has been minimized.

Show comment
Hide comment
@subvertallchris

subvertallchris Nov 7, 2015

Member

Just pushed an update to this that makes plain ole' associations' unique option require one of these same arguments. I'm trying to massage things into place so we can reuse more code, so there will be a few more commits.

Rubocop is barking and it needs a little refactoring. I'm going to extract the specifics of how a relationship is built from QueryProxy into a new class tomorrow.

Member

subvertallchris commented Nov 7, 2015

Just pushed an update to this that makes plain ole' associations' unique option require one of these same arguments. I'm trying to massage things into place so we can reuse more code, so there will be a few more commits.

Rubocop is barking and it needs a little refactoring. I'm going to extract the specifics of how a relationship is built from QueryProxy into a new class tomorrow.

@cheerfulstoic

This comment has been minimized.

Show comment
Hide comment
@cheerfulstoic

cheerfulstoic Nov 8, 2015

Member

Awesome, thanks ;)

I've been thinking about the API and I'm honestly not sure how it will be used, but my thought was that by default you might call creates_unique and it should work like your :none case and then if you specify some arguments those would be equivalent to the {on: [keys]} case. I wonder if the except or :all options would be used much, if at all.

Member

cheerfulstoic commented Nov 8, 2015

Awesome, thanks ;)

I've been thinking about the API and I'm honestly not sure how it will be used, but my thought was that by default you might call creates_unique and it should work like your :none case and then if you specify some arguments those would be equivalent to the {on: [keys]} case. I wonder if the except or :all options would be used much, if at all.

@subvertallchris

This comment has been minimized.

Show comment
Hide comment
@subvertallchris

subvertallchris Nov 9, 2015

Member

I'm ok with it defaulting to :none but I worry that someone who's used to
Cypher's behavior would expect it to behave like :all.

none and on with keys are ones I expect to be the most useful, but I
added the others since it just felt like they should be there and the code
made it easy enough. ;-)

On Sunday, November 8, 2015, Brian Underwood notifications@github.com
wrote:

Awesome, thanks ;)

I've been thinking about the API and I'm honestly not sure how it will be
used, but my thought was that by default you might call creates_unique
and it should work like your :none case and then if you specify some
arguments those would be equivalent to the {on: [keys]} case. I wonder if
the except or :all options would be used much, if at all.


Reply to this email directly or view it on GitHub
#1038 (comment).

Member

subvertallchris commented Nov 9, 2015

I'm ok with it defaulting to :none but I worry that someone who's used to
Cypher's behavior would expect it to behave like :all.

none and on with keys are ones I expect to be the most useful, but I
added the others since it just felt like they should be there and the code
made it easy enough. ;-)

On Sunday, November 8, 2015, Brian Underwood notifications@github.com
wrote:

Awesome, thanks ;)

I've been thinking about the API and I'm honestly not sure how it will be
used, but my thought was that by default you might call creates_unique
and it should work like your :none case and then if you specify some
arguments those would be equivalent to the {on: [keys]} case. I wonder if
the except or :all options would be used much, if at all.


Reply to this email directly or view it on GitHub
#1038 (comment).

@cheerfulstoic

This comment has been minimized.

Show comment
Hide comment
@cheerfulstoic

cheerfulstoic Nov 9, 2015

Member

Regarding people who are used to Cypher, I suspect they're generally going to CREATE UNIQUE with either no or only a small set of properties. The problem is that our API automatically puts in all of the properties/values for you. So thinking about it, I think that even people experienced with Cypher would be surprised at our implementation of creates_unique.

I'm trying to think of what bothers me about the except cases, and I think the problem is that you can have surprising behavior on a team if somebody adds a property. For something that seems like it should be an innocent action, it could have larger consequences. Allowing people to just specify the fields that they want to be used seems safer. Plus it's a cleaner interface because creates_unique without arguments becomes what people would expect it to be (only ever create one relationship between two given nodes) and then you can list out the properties that are important otherwise.

Member

cheerfulstoic commented Nov 9, 2015

Regarding people who are used to Cypher, I suspect they're generally going to CREATE UNIQUE with either no or only a small set of properties. The problem is that our API automatically puts in all of the properties/values for you. So thinking about it, I think that even people experienced with Cypher would be surprised at our implementation of creates_unique.

I'm trying to think of what bothers me about the except cases, and I think the problem is that you can have surprising behavior on a team if somebody adds a property. For something that seems like it should be an innocent action, it could have larger consequences. Allowing people to just specify the fields that they want to be used seems safer. Plus it's a cleaner interface because creates_unique without arguments becomes what people would expect it to be (only ever create one relationship between two given nodes) and then you can list out the properties that are important otherwise.

@subvertallchris

This comment has been minimized.

Show comment
Hide comment
@subvertallchris

subvertallchris Nov 9, 2015

Member

That makes sense. I can remove the Hash aspect of the parameter entirely
and just have it be the Symbol or list of keys. If no argument is given, it
can default to :none, but I'm still not sure it's safe to assume that
everyone will interpret it the way we did. I think we should keep :all as
an option.

On Monday, November 9, 2015, Brian Underwood notifications@github.com
wrote:

Regarding people who are used to Cypher, I suspect they're generally going
to CREATE UNIQUE with either no or only a small set of properties. The
problem is that our API automatically puts in all of the properties/values
for you. So thinking about it, I think that even people experienced with
Cypher would be surprised at our implementation of creates_unique.

I'm trying to think of what bothers me about the except cases, and I
think the problem is that you can have surprising behavior on a team if
somebody adds a property. For something that seems like it should be an
innocent action, it could have larger consequences. Allowing people to just
specify the fields that they want to be used seems safer. Plus it's a
cleaner interface because creates_unique without arguments becomes what
people would expect it to be (only ever create one relationship between two
given nodes) and then you can list out the properties that are important
otherwise.


Reply to this email directly or view it on GitHub
#1038 (comment).

Member

subvertallchris commented Nov 9, 2015

That makes sense. I can remove the Hash aspect of the parameter entirely
and just have it be the Symbol or list of keys. If no argument is given, it
can default to :none, but I'm still not sure it's safe to assume that
everyone will interpret it the way we did. I think we should keep :all as
an option.

On Monday, November 9, 2015, Brian Underwood notifications@github.com
wrote:

Regarding people who are used to Cypher, I suspect they're generally going
to CREATE UNIQUE with either no or only a small set of properties. The
problem is that our API automatically puts in all of the properties/values
for you. So thinking about it, I think that even people experienced with
Cypher would be surprised at our implementation of creates_unique.

I'm trying to think of what bothers me about the except cases, and I
think the problem is that you can have surprising behavior on a team if
somebody adds a property. For something that seems like it should be an
innocent action, it could have larger consequences. Allowing people to just
specify the fields that they want to be used seems safer. Plus it's a
cleaner interface because creates_unique without arguments becomes what
people would expect it to be (only ever create one relationship between two
given nodes) and then you can list out the properties that are important
otherwise.


Reply to this email directly or view it on GitHub
#1038 (comment).

@cheerfulstoic

This comment has been minimized.

Show comment
Hide comment
@cheerfulstoic

cheerfulstoic Nov 9, 2015

Member

Sounds good! The only other thing is that I have the slightest of worries that somebody might have a property called all. So maybe all_properties? I dunno, up to you ;)

Member

cheerfulstoic commented Nov 9, 2015

Sounds good! The only other thing is that I have the slightest of worries that somebody might have a property called all. So maybe all_properties? I dunno, up to you ;)

@subvertallchris

This comment has been minimized.

Show comment
Hide comment
@subvertallchris

subvertallchris Nov 10, 2015

Member

Let's just stick with a hash, {on: [keys]}, to keep things simple and prevent conflicts. It also gives a path for future filter types if something becomes necessary.

Member

subvertallchris commented Nov 10, 2015

Let's just stick with a hash, {on: [keys]}, to keep things simple and prevent conflicts. It also gives a path for future filter types if something becomes necessary.

@subvertallchris

This comment has been minimized.

Show comment
Hide comment
@subvertallchris

subvertallchris Nov 10, 2015

Member

Snuck that in during lunch, will set the default :none tonight and then I think we can merge.

Member

subvertallchris commented Nov 10, 2015

Snuck that in during lunch, will set the default :none tonight and then I think we can merge.

@cheerfulstoic

This comment has been minimized.

Show comment
Hide comment
@cheerfulstoic

cheerfulstoic Nov 11, 2015

Member

Cool ;p

Member

cheerfulstoic commented Nov 11, 2015

Cool ;p

@subvertallchris

This comment has been minimized.

Show comment
Hide comment
@subvertallchris

subvertallchris Nov 11, 2015

Member

Closed by mistake?

Member

subvertallchris commented Nov 11, 2015

Closed by mistake?

@cheerfulstoic

This comment has been minimized.

Show comment
Hide comment
@cheerfulstoic

cheerfulstoic Nov 11, 2015

Member

Yup, sorry about that!

Member

cheerfulstoic commented Nov 11, 2015

Yup, sorry about that!

subvertallchris added a commit that referenced this pull request Nov 12, 2015

Merge pull request #1038 from neo4jrb/creates_unique_with_props
Arguments for ActiveRel.creates_unique, new Association `unique` options

@subvertallchris subvertallchris merged commit 3c3f637 into master Nov 12, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@subvertallchris subvertallchris deleted the creates_unique_with_props branch Nov 12, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment