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

Dj/129 has many #278

Merged
merged 12 commits into from
Sep 9, 2018
Merged

Dj/129 has many #278

merged 12 commits into from
Sep 9, 2018

Conversation

drujensen
Copy link
Member

@drujensen drujensen commented Sep 4, 2018

This PR addresses issue #129 by removing the automatic pluralization (adding s) to the table name. I think the industry agrees this is a bad practice. If you want to pluralize the name, you need to specify the table_name explicitly.

It also changes the has_many relation to remove stripping of the s and add support to allow you to use a type declaration as follows:

  has_many :post

  # pluralization requires providing the class name
  has_many posts : Post

  # or you can provide class name as a parameter
  has_many :posts, class_name: Post

This eliminates the need for adding a inflector to the macro level. This is a breaking change and moves away from the Rails pluralization. You can still pluralize if you want but you need to explicitly specify the type if you do.

This PR also allows you to specify a custom foreign_key:

  has_many :posts, class_name: Post, foreign_key: :custom_id

This PR also refactors the has_one to use the double splat for options so they can be provided in any order.

@c910335
Copy link
Contributor

c910335 commented Sep 5, 2018

I think the syntax is not intuitive. cats is obviously a collection of Cat but not one Cat.
It would be better for me to support the syntax in #129.

has_many cats, class: Cat

@drujensen
Copy link
Member Author

@c910335 I decided to follow the same syntax used in belongs_to but I see how the type declaration may be confusing.

Let me see if I can rework it. I have very little time so this may have to wait for the weekend.

Copy link
Member

@robacarp robacarp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this the way it is. Type declaration is a consistent crystal pattern, and this feels natural to me. Thanks Dru

Granite::AssociationCollection(self, {{model.id.camelcase}}).new(self, {{through}})
end
{% end %}
macro has_many(model, model_name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

model is no longer really descriptive here, is it more of the name of the getter?

@drujensen
Copy link
Member Author

drujensen commented Sep 8, 2018

@robacarp I like the type declaration as well but I agree with @c910335 that has_many cats : Cat is confusing since it really is a collection and not a single instance.

Please re-review this as I have removed the type declaration in favor of class_name. class is a reserved word. Also Rails uses class_name.

I have also implemented foreign_key so now you can have multiple has_many relationships to the same model using different foreign keys.

@drujensen drujensen requested a review from robacarp September 8, 2018 03:47
@drujensen
Copy link
Member Author

@c910335 I'm thinking of adding back the type declaration has_many cats : Cat. The has_many implies its a collection and this reads well to me: has many cats of type Cat. WDYT? Is it too confusing? We can support both syntaxes.

@drujensen
Copy link
Member Author

drujensen commented Sep 8, 2018

I added support for both syntax. Let me know what you think.

@drujensen
Copy link
Member Author

ping!

@drujensen drujensen merged commit 17af8ca into master Sep 9, 2018
@drujensen
Copy link
Member Author

I'm merging this if there is no feedback. Let me know if we need any changes.

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

Successfully merging this pull request may close these issues.

5 participants