Skip to content

Add belongs_to as DSL warning wrapper to has_one to help beginners#710

Merged
lgebhardt merged 3 commits intoJSONAPI-Resources:masterfrom
olleolleolle:feature/belongs-to-as-has-one-alias
Jun 14, 2016
Merged

Add belongs_to as DSL warning wrapper to has_one to help beginners#710
lgebhardt merged 3 commits intoJSONAPI-Resources:masterfrom
olleolleolle:feature/belongs-to-as-has-one-alias

Conversation

@olleolleolle
Copy link
Copy Markdown
Contributor

@olleolleolle olleolleolle commented May 12, 2016

In order to reduce cognitive threshold for new users of JSONAPI::Resource, this PR introduces a method which does the same thing as has_one.

The user can write belongs_to :thing as well as has_one :thing and it means the same thing.

These are equivalent:

class MemberResource < JSONAPI::Resource
  has_one :club
end
class MemberResource < JSONAPI::Resource
  belongs_to :club
end

The user is also warned with a deprecation warning that tries to teach them about why belongs_to is the wrong name.

@fractaledmind
Copy link
Copy Markdown

FWIW, I lost 4+ hours trying to figure out how to wire up an ActiveRecord model that only had belongs_to associations as a JR resource. This would have solved my problem within 10 minutes.

@lgebhardt
Copy link
Copy Markdown
Contributor

@olleolleolle @dgeb and I have been discussing this PR. On one hand we see the value of the alias, especially as evidenced by @smargh's report of wasted time. On the other hand JSON API only supports "has one" relationships with no distinction between has_one and belongs_to. We are trying to keep the Resources class in line with the spec. Exposing the belongs_to relationship at the resource level is exposing the underlying technology a bit in a way we are slightly uncomfortable with.

As a compromise we'd like to see this implemented as a method, instead of an alias, and a deprecation type warning emitted informing the user that a simple has_one is more appropriate. If someone still wants to use belongs_to and doesn't want the message they can override the method on their own base resource class.

What do you say?

@olleolleolle
Copy link
Copy Markdown
Contributor Author

I like that plan! Let's write that message! Here's a first iteration

"DEPRECATION: You exposed a Resource's has_one relationship using the belongs_to class method. We think has_one is more appropriate. If you know what you're doing, and don't want to see this warning again, override the belongs_to class method on your resource."

@lgebhardt
Copy link
Copy Markdown
Contributor

@olleolleolle The message seems good to me. It's long, but that's probably a plus. Maybe we should output the particular resource class the message is coming for so the user can easily find the source.

@olleolleolle
Copy link
Copy Markdown
Contributor Author

@lgebhardt There, edited using the GitHub pen.

@olleolleolle olleolleolle changed the title Add belongs_to as DSL alias to has_one Add belongs_to as DSL warning wrapper to has_one to help beginners Jun 14, 2016
@lgebhardt lgebhardt merged commit 86138a7 into JSONAPI-Resources:master Jun 14, 2016
@lgebhardt
Copy link
Copy Markdown
Contributor

@olleolleolle Thanks! This should help cut down on confusion.

@olleolleolle olleolleolle deleted the feature/belongs-to-as-has-one-alias branch June 14, 2016 15:08
@fractaledmind
Copy link
Copy Markdown

I think the deprecation warning is the perfect solution. Once I figured out the conceptual gap between AR models and JR resources (JR doesn't care about which table the field lives in, only that it can call the association method on the model), I saw how and why has_one and has_many worked. There is no real need for belongs_to except as a conceptual bridge from AR to JR.

@olleolleolle
Copy link
Copy Markdown
Contributor Author

IF we want to reinforce this concept more in the deprecation message, a wiki link, or a README link, could help the user get to that epiphany quicker.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants