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

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

Merged
merged 7 commits into from Nov 12, 2015

Conversation

@subvertallchris
Copy link
Contributor

@subvertallchris 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 Creates unique with props Arguments for ActiveRel.creates_unique Nov 6, 2015
@subvertallchris subvertallchris changed the title Arguments for ActiveRel.creates_unique Arguments for ActiveRel.creates_unique, new Association `unique` options Nov 7, 2015
@subvertallchris
Copy link
Contributor Author

@subvertallchris 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
Copy link
Contributor

@cheerfulstoic 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
Copy link
Contributor Author

@subvertallchris 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
Copy link
Contributor

@cheerfulstoic 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
Copy link
Contributor Author

@subvertallchris 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
Copy link
Contributor

@cheerfulstoic 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
Copy link
Contributor Author

@subvertallchris 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
Copy link
Contributor Author

@subvertallchris subvertallchris commented Nov 10, 2015

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

@cheerfulstoic
Copy link
Contributor

@cheerfulstoic cheerfulstoic commented Nov 11, 2015

Cool ;p

@subvertallchris
Copy link
Contributor Author

@subvertallchris subvertallchris commented Nov 11, 2015

Closed by mistake?

@cheerfulstoic
Copy link
Contributor

@cheerfulstoic cheerfulstoic commented Nov 11, 2015

Yup, sorry about that!

subvertallchris added a commit that referenced this pull request Nov 12, 2015
Arguments for ActiveRel.creates_unique, new Association `unique` options
@subvertallchris subvertallchris merged commit 3c3f637 into master Nov 12, 2015
2 checks passed
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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.