Skip to content

feat: register store_attributes as attributes #32

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

Merged
merged 2 commits into from
May 9, 2025

Conversation

RickCSong
Copy link
Contributor

@RickCSong RickCSong commented Jan 7, 2023

Enables better compatibility with other gems (i.e. StoreModel) which rely on attributes being registered on the model.

Checklist

  • I've added tests for this change
  • I've added a Changelog entry
  • NOT NEEDED - I've updated a documentation (Readme)

@palkan
Copy link
Owner

palkan commented Jan 10, 2023

Enables better compatibility with other gems (i.e. StoreModel)

Could you please provide an example of when it could be useful?

I'm a bit concerned of potential incompatibilities, and in general it's questionable whether we should consider store attributes to be real attributes. I don't know. Maybe, we should make this feature optional and off by default.

@hmasing
Copy link

hmasing commented Jan 17, 2024

Enables better compatibility with other gems (i.e. StoreModel)

Could you please provide an example of when it could be useful?

I'm a bit concerned of potential incompatibilities, and in general it's questionable whether we should consider store attributes to be real attributes. I don't know. Maybe, we should make this feature optional and off by default.

An example is that I would like to store enums as json attributes.

I'd also very much like to be able to query on these attributes, fwiw.

@palkan palkan force-pushed the master branch 2 times, most recently from f33a4df to 9109a92 Compare March 5, 2024 02:31
@Petercopter
Copy link

Hey cool, this is exactly what I need. Right now we are using https://github.com/madeintandem/jsonb_accessor, but we need to move away from it because it's not longer maintained.

We are iterating over all attributes on a model in order to do some token replacement in text.

page.attributes.each do |attr|
  find_and_replace_tokens
end

That's my use case for needed data attributes to respond as attributes.

Thanks!

@palkan
Copy link
Owner

palkan commented Apr 18, 2024

@hmasing @Petercopter Thanks for the details!

Let's then proceed with this feature but make it opt-in for now; we can added a setting similar to store_attribute_unset_values_fallback_to_default:

class Post < ApplicationRecord
  self.store_attribute_register_attributes = true
  
  store_attribute :extra, :published_at, :datetime
end

Post.new.attributes.include?("published_at") #=> true

@RickCSong Will you be able to add this change? If not I (or, maybe, someone else) can pick up from here, no worries. Thank you!

@palkan palkan added the stale label Dec 13, 2024
@RickCSong RickCSong closed this Jan 8, 2025
@RickCSong RickCSong reopened this Apr 23, 2025
@4ndypanda
Copy link

Hey @palkan you haven't done this right? @RickCSong is a buddy and we can find someone to add the change you suggested!

@RickCSong RickCSong force-pushed the master branch 3 times, most recently from 06838ae to dfe7aef Compare April 23, 2025 07:15
Enables better compatibility with other gems (i.e. StoreModel) which
rely on attributes being registered on the model.
@palkan
Copy link
Owner

palkan commented Apr 28, 2025

Hey @4ndypanda,

you haven't done this right?

Right. Feel free to pick up the PR!

@n-ivan
Copy link

n-ivan commented May 7, 2025

Hey @4ndypanda,

you haven't done this right?

Right. Feel free to pick up the PR!

Hey @palkan the @RickCSong added the change you suggested to the PR - this is ready for rereview let us know if there's any other changes you need

Copy link
Owner

@palkan palkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks good

@palkan palkan merged commit 8335b7e into palkan:master May 9, 2025
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants