Feature request: better custom type representation
Closed this issue · 7 comments
Describe your problem here.
Commands
We've been using annotate for some time alongside with custom AR types (via ActiveRecord::Type.register
). Unfortunately, annotate was serializing values of these types into extremely long strings:
# currency :string default(#<Money::Currency id: usd, priority: 100, symbol_first: false, thousands_separator: ., html_entity: ₽, decimal_mark: ,, name: United States Dollar, symbol: $, subunit_to_unit: 100, exponent: 2, iso_code: RUB, iso_numeric: 643, subunit: Cent, smallest_denomination: 1, format: %n %u>), not null
We workaround the issue by monkey-patching ``AnnotateModels.quote`:
....
when BigDecimal then value.to_s("F")
when Array then value.map { |v| quote(v) }
when Locale then value.to_s.inspect # custom types start here
when Region then value.to_s.inspect
when Billing::Plan then value.to_s.inspect
so annotations were good to look at:
# currency :string default("USD"), not null
We are looking forward migration to annotaterb so we'd also want to get rid of that monkey-patch. Perhaps an addtional configuration option would be sufficient?
Version
- annotaterb version: 4.6.0
- rails version: 7.1.3.2
- ruby version: 3.3.0
Hi @viralpraxis, thanks for checking out the gem and adding a featuring request!
Maybe we can brainstorm a solution together. I think adding a configuration option that takes a list of classes/types that gets mapped to value.to_s.inspect
could work. I'm curious if there may be other custom mappings that might be desired as well.
On the other hand, adding some way to load some monkeypatch could bridge the gap for folks looking to migrate from the old annotate gem. So I can see value there.
Thoughts?
I think adding a configuration option that takes a list of classes/types that gets mapped to value.to_s.inspect could work
I'm pretty sure this approach would do the trick. One thing I'm not sure about is how specific the configuration option should be. It might be as simple as
classes_with_to_s_representation:
- Locale
- Region
or it might be more flexible:
custom_type_default_value_representation:
- class: "Locale"
methods: ["to_s", "inspect"] # value.to_s.inspect
- class: "Region"
methods: "to_s" # value.to_s
No matter what we choose, we'll only have to slightly enhance DefaultValueBuilder#quote
to take these mappings into account
@drwl Any thoughts? I'd be happy to contribute and implement any solution that will fix the issue.
@drwl Any thoughts? I'd be happy to contribute and implement any solution that will fix the issue.
Apologies for the delay. I agree with your thoughts. I think the first approach could fit both the config file as a CLI option more easily. Perhaps we could make the key something like classes_default_to_s
? Regardless, if you want to put up a PR I'd be more than happy to review once it's ready.
@viralpraxis I pushed up 4.7.0 to Rubygems if you want to give it a try. Feel free to reopen this if it doesn't work and thanks again for your help!
@drwl with v4.7.0 everything works just as we expect, thank you for creating and maintaining this fork! 💎
By the way, looks like the changelog is corrupted a little bit:
Thanks for pointing that out, I'll fix it