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

🌈The klass parameter 'inject_into_class' should be given a string type.(also inject_into_module) #752

Merged
merged 1 commit into from
May 27, 2021

Conversation

ratovia
Copy link
Contributor

@ratovia ratovia commented Apr 13, 2021

I think that in most cases the target klass is not defined in a file that uses inject_into_class.
So it is more friendly to set the default comment to string type.
as-is

inject_into_class "xxx.rb", ApplicationController, "Lorem"

to-be

inject_into_class "xxx.rb", "ApplicationController", "Lorem"

I think it's more realistic for test specs to delete the empty class definition in the first line.
Also about the inject_into_module.

@ratovia ratovia changed the title The klass parameter 'inject_into_class' should be given a string type.(also inject_into_module) 🌈The klass parameter 'inject_into_class' should be given a string type.(also inject_into_module) Apr 13, 2021
@xjunior
Copy link

xjunior commented May 26, 2021

Looks good, but I think it's better if both are allowed. Since the implementation already allows that, you could just add another test to make sure strings are also supported. And +1 to update the docs.

@rafaelfranca
Copy link
Member

That works by "mistake" today and I prefer if we don't advertise it. I'll merge this as-is

@rafaelfranca rafaelfranca merged commit 2d25975 into rails:master May 27, 2021
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