Infer association class name when not provided

[wfleming]

Defining setters & getters blindly without knowing if a class name was
provided doesn't seem great. Needing to provide a class name explicitly
when in many cases it is easily inferrable also seems odd.

This does a simple inferring of the class name for an association based
on the association name.

I think this simple enough to still be "mini". If we don't want to add
this, though, we probably shouldn't define the setter & getter methods
when the class_name option isn't provided.

cc @codeclimate/review

[pbrisbin]

LGTM. Are there are gotchas related to namespaces?

[maxjacobson]

Code & premise LGTM. As I said somewhere else, we already do some inference to map the class name to the collection name, so I think this is in-scope.

[wfleming]

Are there are gotchas related to namespaces?

Good catch: Yes, there would be. e.g.

module Foo
  class Thing
    belongs_to :bar
  end

  Bar = Class.new(Object)
end

Will end up looking for ::Bar, not Foo::Bar.

I think it's an open question of what the behavior should be.

1. Leave it as-is. This is the simplest option, and will work sanely in the majority of cases.
2. Assume any inferred class_name would be in the same module as the enclosing class. E.g. infer Foo::Bar. This will be useful in some cases, but may be surprising/confusing in others. E.g. belongs_to :user is the kind of thing you're likely to see in lots of classes in lots of modules, and wouldn't work without an explicit class_name for this logic.
3. Check for class existence at time of determining the class_name, and walk up the module ancestry from the module of the calling class. This would work for both Foo::Bar and ::User without needing to be explicit, but would complicate implementation quite a bit I think.

Personally I think option 1 seems appropriate: it's simple & covers common cases. Option 2 seems to trade confusion in some cases for different confusion in other common cases. Option 3 goes too far to be suitable for the minimalist nature of this library (and adds some dependencies like the associated class being loaded/defined at association definition time). WDYT?

[pbrisbin]

FWIW, option 2 is what I'd expect if I were using this library and guessing at the behavior. I think it'd be reasonable to assume namespace would be respected and have to add class_name: "::User" to clarify my intent in that case.

I'm fine with option 1, but it does mean we're not getting this benefit in most of our services, so I wonder if it's worth adding at all.

[wfleming]

You make a good point about our services. I was thinking of Rails as the "default case", but that's not really our dominant case. I'll update for approach 2 this afternoon if I can.

[wfleming]

@pbrisbin this has been updated to take approach 2.

[pbrisbin]

LGTM

[pbrisbin]

Just noting that this would be considered a breaking change if we were doing actual versioned releases (I think?). So the timeline discussed in #27 may no longer be correct.

[wfleming]

Just noting that this would be considered a breaking change

Would it? Any existing code that specifies the class name is unaffected, and any existing code that doesn't specify the class name would be nil-erroring all over the place if you used the setters/getters (without this change). I feel like that's backwards compatible?

[pbrisbin]

I feel like that's backwards compatible?

Sure, I was unsure. What you say makes sense. /cc @maxjacobson

[maxjacobson]

I think it's a grey area and it depends on how strict you want to be. I think if we're going to be as strict as possible, it's a change in behavior and we need to make a major version bump. If we're going to be medium strict, we can characterize it as adding functionality, and call it a minor version bump. If we're not being strict, we can characterize it as a tiny change, because for the users not specifying a class_name who would like to specify a different class than we infer, it's just changing from broken to different-broken, as you said. I don't think we're in a position where we need to be majorly strict, so I would prob characterize it as a minor-release-worth-change.

[dblandin]

Any existing code that specifies the class name is unaffected, and any existing code that doesn't specify the class name would be nil-erroring all over the place if you used the setters/getters (without this change). I feel like that's backwards compatible?

Based on this description, I would consider the change a bug fix or a feature addition (depending on how you define "bug"), worthy of a patch or minor version bump (if we were already versioning). I don't think this qualifies as breaking backwards-compatibility.