Test on Ruby 3 #612
Test on Ruby 3 #612
Conversation
| @@ -78,7 +78,7 @@ class Engine < ::Rails::Engine | |||
| full_name: N_("Hostname facts"), | |||
| description: N_("List of facts to use for the hostname (first wins)") | |||
|
|
|||
| validates "discovery_hostname", presence: true | |||
There was a problem hiding this comment.
It surprises me this is needed. Can't we look at the validates method to see if presence: true can be made valid?
There was a problem hiding this comment.
Only if we change its signature: https://github.com/theforeman/foreman/blob/develop/app/registries/foreman/setting_manager.rb#L58
I mean, currently validates expects 2 arguments. Due to new Ruby kwargs, they are not considered as an implicit Hash argument.
As a solution I'd suggest to finally use kwargs as Ruby wants or we could make second (validations) parameter to be optional (validations = {}).
There was a problem hiding this comment.
It's supposed to map to ActiveModel::Validations::ClassMethods.validates and that uses def validates(*attributes). I'm not sure that is a great API.
So I think adjusting the signature is the right thing. Just not 100% sure what it should be. Perhaps:
def validates(name, **validations)And then special case if :proc is in validations.
There was a problem hiding this comment.
That could be, but since the old signature accepts proc as an argument (not nested under :proc key), we would still need to update places which use that. I think we would need to adjust plugins anyway if we change the signature.
There was a problem hiding this comment.
Perhaps def validates(name, proc = nil, **validations) would work?
|
I think #622 replaces this. |
|
Closing in favor of #622 |
Please, don't merge just yet. Foreman on Ruby 3.0 is still in progress.