-
-
Notifications
You must be signed in to change notification settings - Fork 50
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
Make compatible with Mobility translation gem #34
Conversation
I'm not too keen on this. Can you please explain why this isn't a bug in the mobility gem? Strip attributes wasn't doing anything that any ActiveRecord library might do. I.e. Strip attributes is just adhering to the What am I missing? |
@shioyama, what do you think about this? |
Further reference material showing Feel free to chime in with more information about why you think |
Sorry I'm actually at RubyKaigi right now preparing for a talk, haven't had time to respond. I don't necessarily think this is the fault of strip_attributes, so no issue with closing this, but I will explain what the issue is later when I have a chance. |
Any news on this? I'm still using my own fork to circumvent this problem. It would be nice to switch back to the official gem again. |
I'm not planning to do anything about this since I don't consider it a bug (either in this gem or in Mobility). Translated attributes are not attributes, technically. They are mappings to attributes stored in different places. If I go ahead and override Bottom line: there is no bug here. However, I will point out that |
Yeah, strip_attributes has been around for a while now, and when I first wrote it, different conventions existed in Rails than they do now. Also, there are newer API endpoints that can now be considered too. e.g. I might consider using the new attributes API instead of a class Strippable < ActiveRecord::Type::String
def cast_value(value)
value.to_s.strip
end
end
class Something < ActiveRecord::Base
attribute :title, Strippable
end |
The attributes API is great for stuff like value types, etc. but for something like just stripping whitespace wouldn't just basing off a basic getter/setter API work? I mean, override |
@shioyama All valid things to consider, and I will consider that when I later look into this. This gem is already dependent on ActiveModel, because it can automatically inspect for all possible attributes on the model. I’d like to maintain that behavior regardless. It’s possible that custom types is a bad idea, because it might prevent an application from defining their own custom types unless they were willing to inherit from some Strippable type. That would give a nod more to the overridden setters, but overridden setters won’t handle assignment while using write_attribute. |
Actually, rethinking this, I think you're probably right to use |
jsonb_accessor and attr_json are two gems that provide alternate implementations to have activerecord attributes that really serialize to keys in a json column. strip_attributes does not work with either one as is -- but would if it used (Some of the commentary above talks about "ActiveModel attributes" -- but really |
I also stumbles across this problem. I can only support the statements made by other comments. 👍 I think that this could be a good addition for the gem as suggested by the others. In this case, I did not need a full-fledged ActiveRecord model and just used an ActiveModel object, see below. class InitialOffer
include ActiveModel::Model
include ActiveModel::Attributes # <= this is needed to make the method `#attributes` available
include ActiveModel::Validations::Callbacks # as explained in the README this is needed to run the validation
attribute :attr1
attribute :attr2
strip_attributes only: %i[
attr1
attr2
]
end When providing a value with just whitespaces then it crashes because strip_attributes would convert blank values to InitialOffer.new(attr1: " ").validate This leads to the following error /home/app/.gem/3.2.0/gems/activemodel-7.0.8/lib/active_model/attribute_methods.rb:450:in `method_missing': undefined method `[]=' for #<InitialOffer:0x0000ffff720ec230 ... |
https://github.com/shioyama/mobility/ is a very nice translation gem. Sadly it doesn't play well with
strip_attributes
. This little change fixes it. Tests pass. All fine in my opinion.See shioyama/mobility#97.