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

Raise error uncountable slot names in plural slots #1904

Merged

Conversation

reeganviljoen
Copy link
Collaborator

@reeganviljoen reeganviljoen commented Nov 8, 2023

closes #1903

What are you trying to accomplish?

This Attempts to fix the bellow bug by raising an error when a plural slot is used with an uncountable name

What are you trying to accomplish?

I am submitting this PR to report an error related to naming a slot with an uncountable name in my component. I attempted to create a slot named "series," but it resulted in an error. This error is due to the fact that the name "series" is uncountable and does not follow Rails convention.

With reference to the uncountable words defined in Rails' inflections list, I expected to use the name "series" for my slot. However, Rails expects a pluralized slot name, leading to this error.

Here is the specific error message I encountered:

Error:
SlotableTest#test_slot_with_unplurialized_name:
NoMethodError: undefined method `with_serie' for #<NamingSlotTestComponent:0x000000010fd97c30 @_routes=nil>
Did you mean?  with_series

What approach did you choose and why?

I suggest creating a more explicit error message or exploring alternative ways to resolve this issue.

@reeganviljoen reeganviljoen changed the title Rv fix slot uncountable Raise error uncountable slot names in plural slots Nov 8, 2023
@reeganviljoen
Copy link
Collaborator Author

@hchtlz I credited you with me in the change log because you discovered this issue and gave us a failing test to work with

@hchtlz
Copy link

hchtlz commented Nov 8, 2023

@reeganviljoen Thank you for the credit in the changelog. I was delighted to contribute to solving this issue

Copy link
Contributor

@BlakeWilliams BlakeWilliams left a comment

Choose a reason for hiding this comment

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

Great fix, thanks!

@BlakeWilliams
Copy link
Contributor

Actually, I think this would have been problematic in a previous API. Is that still the case for this iteration of slots?

Would we expect has_many :series to result in:

Component#series # gets the slots
Components#with_series # sets the slot

Which seems fine, unless I'm missing something here.

In the example failure we shouldn't be calling with_serie since that's not a valid word. So I guess, do we need this check?

@BlakeWilliams
Copy link
Contributor

Ahhhh, we do still need this because of the extra methods we define:

define_method :"with_#{slot_name}" do |collection_args = nil, &block|
collection_args.map do |args|
if args.respond_to?(:to_hash)
set_slot(slot_name, nil, **args, &block)
else
set_slot(slot_name, nil, *args, &block)
end
end
end
define_method slot_name do
get_slot(slot_name)
end
define_method "#{slot_name}?" do
get_slot(slot_name).present?
end

@BlakeWilliams BlakeWilliams merged commit 1680c6f into ViewComponent:main Nov 10, 2023
23 checks passed
claudiob pushed a commit to claudiob/view_component that referenced this pull request Dec 22, 2023
* Raise error when uncountable slot names are used in `renders_many`

    *Hugo Chantelauze*
    *Reegan Viljoen*
claudiob pushed a commit to claudiob/view_component that referenced this pull request Jan 3, 2024
* Raise error when uncountable slot names are used in `renders_many`

    *Hugo Chantelauze*
    *Reegan Viljoen*
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