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

Fix association names when scaffolding join models if they don't match the table name #848

Merged
merged 12 commits into from
Jun 13, 2024

Conversation

jagthedrummer
Copy link
Contributor

@jagthedrummer jagthedrummer commented Jun 5, 2024

Fixes the association naming problem in: bullet-train-co/bullet_train#1508

I'm going to create a separate issue about the incorrect valid_ method that's added on one side of the association.

@jagthedrummer jagthedrummer force-pushed the jeremy/join-model-attribute-naming branch from ab7d656 to 057e7ef Compare June 5, 2024 21:05
@jagthedrummer jagthedrummer changed the title WIP: join model attribute naming Fix association names when scaffolding join models if they don't match the table name Jun 6, 2024
@jagthedrummer jagthedrummer marked this pull request as ready for review June 6, 2024 17:56
@@ -58,6 +58,20 @@ def association_class_name
name.split("_id").first
end

def association_name
association_class_name.tableize
Copy link
Contributor

Choose a reason for hiding this comment

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

tableize will also pluralize, which I don't know if we want for belongs_to/has_one associations. For those, we'd probably want underscore.tr("/", "_").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 Hmm, good point. Currently I think this method is only being called in places where we have a has_many or has_many through: relationship, but I can see how this would be confusing and potentially lead to problems down the road. I'll clean it up a bit.

Comment on lines +65 to +74
def class_name_matches?
# if namespaces are involved, just don't...
# TODO: I'm not entirely sure that extracting this conditional was the right thing to do.
# Are there scenarios where we want to assume a match even when namespaces are involved?
if options[:class_name].include?("::")
return false
end
name_without_id.tableize == options[:class_name].tableize.tr("/", "_")
end
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the cases that this logic is for, I'm having a bit of a hard time understanding it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kaspth This is used in situations where you're scaffolding a relationship with a name that doesn't match the name of the table/model you're relating to. See the main description in this issue for an example: bullet-train-co/bullet_train#1508 (comment)

@kaspth
Copy link
Contributor

kaspth commented Jun 12, 2024

Ok, there's a whole bunch of code that I haven't really looked at and that it'd take me some more time to dive into, so feel free to forward without me :)

@jagthedrummer jagthedrummer force-pushed the jeremy/join-model-attribute-naming branch from be692ca to 52dc390 Compare June 13, 2024 20:07
@jagthedrummer jagthedrummer merged commit 5808218 into main Jun 13, 2024
30 checks passed
@jagthedrummer jagthedrummer deleted the jeremy/join-model-attribute-naming branch June 13, 2024 20:45
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.

2 participants