Conversation
treagod
force-pushed
the
query-set-enum-input
branch
from
5d64bf3 to
ad5aa7d
Compare
| def initialize( | ||
| @id : ::String, | ||
| enum_values : Array(::String), | ||
| @enum_type : ::Enum.class | Nil = nil, |
There was a problem hiding this comment.
Why is this nilable? Technically, it shouldn't be possible to have an enum field without an actual enum class.
| enum_values : Array(::String), | ||
| @enum_type : ::Enum.class | Nil = nil, |
There was a problem hiding this comment.
I would argue that if we end up specifying the actual enum class to this method, then we could refactor the implementation of this field so that we only specify an @enum_class argument (instead of enum_type and enum_values, which do not appear useful given that values can be inferred from the type directly).
| values.each do |key, value| | ||
| next unless value.is_a?(Field::Any | Model) | ||
| sanitized_values[key.to_s] = value | ||
| accepted, normalized_value = normalize_set_field_value(value) |
There was a problem hiding this comment.
Can we avoid using method overloading for this? This makes the code more difficult to maintain for very little gains I believe. I standard case/when would work perfectly fine here:
values.each do |key, value|
sanitized_value = case value
when Field::Any, Model
value
when ::Enum
value.to_s
else
next
end
sanitized_values[key.to_s] = value
end| values.each do |k, v| | ||
| case v | ||
| when Field::Any, DB::Model | ||
| update_hash[k.to_s] = v | ||
| when ::Enum | ||
| update_hash[k.to_s] = normalize_enum_update_value(k, v) | ||
| else | ||
| raise Errors::UnexpectedFieldValue.new( | ||
| "Value '#{v.inspect}' cannot be used in update queries" | ||
| ) | ||
| end | ||
| end |
There was a problem hiding this comment.
I am not convinced that we should introduce this type of logic at this level. The Query#update_with method already calls #to_db for each of the fields corresponding to the values specified as input. I would argue that we should try to reuse this as much as possible instead of replicating some of this logic at the set level (whose responsibility is mainly to enable the query set interface and to leave everything related to data preparation to the query abstraction).
| field = begin | ||
| M.get_field(field_name) | ||
| rescue Errors::UnknownField | ||
| nil | ||
| end | ||
| return value.to_s if field.nil? || !field.is_a?(Field::Enum) | ||
|
|
||
| field.as(Field::Enum).to_db(value).as(Field::Any) |
There was a problem hiding this comment.
Additionally, outside of relation fields, we should avoid making assumptions regarding the type of fields being manipulated. As such, we should not explicitly check that the manipulated field is an enum field, ideally.
|
|
||
| localized_format_index = 0 | ||
| fallback_format_index = 0 | ||
| format = I18n.t("marten.schema.field.date.input_formats.#{localized_format_index}") |
There was a problem hiding this comment.
This can be removed from this PR (I believe this was already fixed in main).
|
|
||
| localized_format_index = 0 | ||
| fallback_format_index = 0 | ||
| format = I18n.t("marten.schema.field.date_time.input_formats.#{localized_format_index}") |
2 participants
This PR adds the possibility to use an enum value to set a field value