drwl/annotaterb

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: &#x20BD;, 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
drwl commented

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 commented

@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.

drwl commented

@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:

2024-03-28_02-20

drwl commented

Thanks for pointing that out, I'll fix it