-
Notifications
You must be signed in to change notification settings - Fork 58
Add prefix to attribute definition #43
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
base: master
Are you sure you want to change the base?
Conversation
LGTM |
|
||
it 'defines accessors' do | ||
expect(model).to respond_to :preferences_language | ||
expect(model).to respond_to :preferences_language= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a test to make sure language
and language=
are not defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I need to look into how the attribute definition works as well. The values does not seem to be set correctly. I'll ping back once I've figured it out.
a9ea2c5
to
97478f7
Compare
Typed Store looks great! It would really be a good fit to simplify something my team did a long time ago. The one thing that's stopping me from using it is the Is there anything I can do to help get this PR out and get the string/symbol prefix complete? Here's the relevant ActiveRecord code. https://github.com/rails/rails/blob/de53ba56cab69fb9707785a397a59ac4aaee9d6f/activerecord/lib/active_record/store.rb#L112-L120 Thanks! |
This PR is very outdated, but if you feel like submitting a similar one that is rebased, I'll review it. |
Sounds good. Thanks, Jean! |
I was also looking for this functionality as well, so I went ahead and rebased and put up a PR at #91 |
Whilst adding accessor: false, I thought it would be convenient to add prefix to attributes as well. It also fixes #7.
Thoughts?
@byroot @rafaelfranca