Skip to content
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

Feature request: better custom type representation #97

Closed
viralpraxis opened this issue Mar 20, 2024 · 7 comments · Fixed by #98
Closed

Feature request: better custom type representation #97

viralpraxis opened this issue Mar 20, 2024 · 7 comments · Fixed by #98

Comments

@viralpraxis
Copy link
Contributor

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
Copy link
Owner

drwl commented Mar 22, 2024

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?

@viralpraxis
Copy link
Contributor Author

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

@viralpraxis
Copy link
Contributor Author

@drwl Any thoughts? I'd be happy to contribute and implement any solution that will fix the issue.

@drwl
Copy link
Owner

drwl commented Mar 25, 2024

@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 drwl closed this as completed in #98 Mar 27, 2024
drwl pushed a commit that referenced this issue Mar 27, 2024
Adds configuration option to convert custom class types to their string representation.

Will resolve #97
@drwl
Copy link
Owner

drwl commented Mar 27, 2024

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

@viralpraxis
Copy link
Contributor Author

@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
Copy link
Owner

drwl commented Mar 28, 2024

Thanks for pointing that out, I'll fix it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants