Add: has_one(chainable: true) option

[jorroll]

By default, model.has_one calls .first on the result. If the result is nil, this means the method is not chainable. I'd like to have the option of getting an association proxy back from a has_one getter. Having dug into the source, I see that has_one methods actually take arguments and options, and one of those options (rel_length) can return a chainable result, but

1. the method name rel_length is not an intuitive argument to pass if you specifically want a chainable result.
2. rel_length must have a non-integer value to return an association proxy (e.g. rel_length: '') which adds to the complexity / is ugly.

This pull introduces/changes:

• adds a chainable: true option to has_one methods that returns an association proxy instead of an ActiveNode object

comment.post(chainable: true).author

Pings:
@cheerfulstoic
@subvertallchris

[jorroll]

Currently, in the docs this see also block links to nothing. I've updated it with appropriate links to the API on rubydoc.

[cheerfulstoic]

Thanks! I had actually written some code a while back to turn the YARD documentation into rst files for readthedocs (you can see the rst formatting on rubydoc.info), but re-implementing rubydoc.info was probably largely a waste of my time ;) . I think these were links to that internal API documentation, but glad to have these fixed now

[jorroll]

Rubocop was failing this with PerceivedComplexity as too high. I didn't know what to do with that, so I suppressed the warning to make it pass 😕 ...

[cheerfulstoic]

It could probably just do with an extraction of a method. I'll fix it and link to the commit here

[cheerfulstoic]

Meh, not the best: 1122ee7

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@bdf8441). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #1422   +/-   ##
=========================================
  Coverage          ?   96.86%           
=========================================
  Files             ?      205           
  Lines             ?    12455           
  Branches          ?        0           
=========================================
  Hits              ?    12065           
  Misses            ?      390           
  Partials          ?        0

Impacted Files	Coverage Δ
lib/neo4j/active_node/has_n.rb	98.29% <100%> (ø)
spec/e2e/has_one_spec.rb	100% <100%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bdf8441...faa7440. Read the comment docs.

[cheerfulstoic]

I could see the use for this, but what about:

comment.association_proxy(:post).author

I think I like your solution better because it's a bit more obvious, but I wanted to mention that

[cheerfulstoic]

We could even probably alias association_proxy as something like start_chain. I'm good either with something like that or this PR

[jorroll]

I find having the method name describe the function rather then the argument describe the function to be more readable and intuitive. The decision to have the result wrapped vs unwrapped seems to be fundamentally a param choice, rather than a method choice.

For example:

reply.comment(chainable: true).post(chainable: true).author(chainable: true).posts.comments

vs

reply.start_chain(:comment).start_chain(:post).start_chain(:author).posts.comments

[jorroll]

Especially when I'm already conditioned by Neo4j to see an association chain as model.path.path.path if it suddenly became model.method(:path).method(:path).method(:path) it feels like its breaking convention.

[cheerfulstoic]

Yeah, I actually agree a lot. We actually have two options for specifying OPTIONAL MATCH for associations:

object.association(optional: true)
object.optional(:association)

I prefer the first one as well. I'll have a look through this PR this, thanks!

[cheerfulstoic]

Looks great. Since this isn't a bugfix but a backwards compatible change I'll release this in 8.3.0, though let's get #1414 and #1419 done and we can release them all together.

#1419 is indeed a breaking change, though I doubt it will affect anybody so I'm tempted to just put it into 8.3 (especially since we have good exceptions around it)

[cheerfulstoic]

Thanks! I had actually written some code a while back to turn the YARD documentation into rst files for readthedocs (you can see the rst formatting on rubydoc.info), but re-implementing rubydoc.info was probably largely a waste of my time ;) . I think these were links to that internal API documentation, but glad to have these fixed now

[cheerfulstoic]

It could probably just do with an extraction of a method. I'll fix it and link to the commit here

[cheerfulstoic]

I think this would be a bit better as:

expect(unsaved_node.parent(chainable: true)).to be_a Neo4j::ActiveNode::HasN::AssociationProxy

I can make that change

[cheerfulstoic]

a34cb4c

[jorroll]

I imagine the semantic versioning standard was created specifically to prevent an update like this from being released as a minor point increase. Major updates pretty obviously demand someone's attention. I imagine the little updates are the ones that break something because no one expects them to. Definitely understand the temptation, but can't say I support it.

[cheerfulstoic]

Yeah, that's certainly fair, and I can change it to 9.0.0 for these changes. I have a couple of thoughts about it, though;

Since I just support the gems in my spare time, I generally don't have the ability to keep track of a number of changes which would get bundled into a major / minor release. It's much easier to simply upgrade the version whenever any change, big or small, comes in. If that's a breaking change, that means that the version could start to go up to 10.x.x, 20.x.x, and onward. It may be that this is fine and I just need to do that, but...

I also worry about having people upgrade major versions and them being completely non-events because something that was released was technically a breaking change, but only affects users who would be using the feature in a non-typical way. That may lead them to see things as being broken in the other direction (that major versions are upgraded without much reason). This fear probably comes from a notion that big / major version number changes are supposed to be a "big deal" and generally come with big changes (each version of iPhone/iOS, HTTP 1 to HTTP 2, Web 2.0, etc...)

But on balance I think it's probably better to upgrade versions strictly based on semver. If it becomes something that people are mentioning I could probably have a note in the README to explain

[jorroll]

Yeah I get that. If you wanted, I think it would be possible to achieve both by creating a duel versioning scheme (e.g. Google Chrome does that). The semver version is what people would spec against. The branding(?) version is what people could look at to get a sense of how large the update is. In the readme you might put:

Neo4jrb 3

semver 13.4.3

Of course you'd need to explain this.

And ya, large open source projects with paid maintainers will manage releases to limit the semver point increase, but I think its totally ok for volunteer projects to ignore that. I don't see anything wrong with the version blowing up to 20/30/40.x.x -- and if devs do have a problem with it, I imagine they'll give feedback.

At its core: ya, as a dev its annoying when a dependency demands your attention during an update and forces you to look at the changelog, but for breaking changes like this, people really should look at the changelog.

This being said, you're doing a kick-ass job managing this repo as is, so ultimately, do what you gotta do.

[cheerfulstoic]

Yup, thanks ;) I think I'll just go with the simplest "upgrade whatever is appropriate" for each change for now, at least ;)

I just realized I didn't release this as 8.3.0 previous, so I just did that now