-
Notifications
You must be signed in to change notification settings - Fork 43
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
fixes #6158 - only dump defaults for param defaults starting $
#194
Conversation
66acf98
to
ea72ae7
Compare
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.
looks good and works fine, just two nits to consider
end | ||
|
||
def default=(default) | ||
default = nil if ['UNSET', :undef].include?(default) |
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.
could you extract ['UNSET', :undef]
from here and manifest_default=
to some constant, e.g. UNSETTING_VALUES so they do not get out of sync?
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.
Extracted to UNSET_VALUES
.
end | ||
|
||
def manifest_default_params_variable | ||
manifest_default[1..-1] if manifest_default.to_s.start_with?('$') |
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.
if dump_default_needed?
or rename/alias the method so it makes sense in this context
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.
Used dump_default_needed?
.
Refactors how defaults from the manifest and the defaults dump are stored, keeping them in separate variables and expecting any params with other variables as the default to have `$` prefixes. Only params with a `$` default prefix will be dumped, so any other default string values will not be dumped and are persisted correctly.
ea72ae7
to
5dae08a
Compare
PR updated with the suggested changes. |
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.
Thanks @domcleal, merging.
Refactors how defaults from the manifest and the defaults dump are
stored, keeping them in separate variables and expecting any params
with other variables as the default to have
$
prefixes.Only params with a
$
default prefix will be dumped, so any otherdefault string values will not be dumped and are persisted correctly.
Needs theforeman/kafo_parsers#31, I can increase the min kafo_parsers version in the gemspec if/when that's released.