-
Notifications
You must be signed in to change notification settings - Fork 983
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
Refs #24281 - Refactor class methods of create and create! #5847
Conversation
Issues: #24281 |
There were the following issues with the commit message:
If you don't have a ticket number, please create an issue in Redmine. More guidelines are available in Coding Standards or on the Foreman wiki. This message was auto-generated by Foreman's prprocessor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the nitpicky comments @ik5 , in general 👍 if tests pass, could you update it? Thank you!
app/models/setting.rb
Outdated
else | ||
create_existing(s, opts) | ||
s = Setting.find_by_name(opts[:name].to_s) | ||
if s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick, return create_existing(s, opts) if s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
app/models/setting.rb
Outdated
@@ -198,28 +198,29 @@ def parse_string_value(val) | |||
true | |||
end | |||
|
|||
def self.create(opts) | |||
# in order to avoid code duplication, this method was introduced | |||
def self.create_find_by_name(opts, &block) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may remove the &block
, implicit is way faster - you may get some background here ruby/ruby#1240
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keep on forgetting that :( thanks
@dLobatog what do you think of this now? |
Thanks @ik5 ! |
@ik5 please don't use Refs in git commit message, it only links the PR in redmine but does not close the issue. |
No description provided.