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

Store options in inheritable class attributes. #227

Merged
merged 2 commits into from
Aug 13, 2018
Merged

Store options in inheritable class attributes. #227

merged 2 commits into from
Aug 13, 2018

Conversation

jnfeinstein
Copy link
Collaborator

This fixes issue #226. Underlying cause is that the options were
being keyed on collection name, which is able to be changed.
This change stores the options on the class itself where it will
always be available regardless of collection name.

@jnfeinstein
Copy link
Collaborator Author

@dblock this fixes #226. Somehow all the tests pass. Wicked!

@jnfeinstein
Copy link
Collaborator Author

Rubocop is throwing two errors on lines I didn't change. Not sure what to do with those.

@dblock
Copy link
Collaborator

dblock commented Aug 11, 2018

For rubocop run rubocop -a ; rubocop --auto-gen-config.

@dblock
Copy link
Collaborator

dblock commented Aug 11, 2018

This is straightforward, I love it. It's a great incremental step on top of #208, I should have totally done that.

@dblock
Copy link
Collaborator

dblock commented Aug 11, 2018

Optional/next steps: do we need trackable_classes at all at this point? We can iterate over mongoid models looking for any that implement trackable history to reimplement reset!, which I think is only used for tests anyway.

Make sure to update CHANGELOG and fix rubocop and it's good to go!

This fixes issue #226.  Underlying cause is that the options were
being keyed on collection name, which is able to be changed.
This change stores the options on the class itself where it will
always be available regardless of collection name.
This should reduce build times.
@jnfeinstein
Copy link
Collaborator Author

@dblock this PR has been updated per your suggestions.

I added a commit that allows Travis to use the latest prebuilt ruby in the 2.3 tree. This knocks off ~18s per build. I can open a separate PR for this if that's preferred.

@dblock dblock merged commit 7635d36 into mongoid:master Aug 13, 2018
@dblock
Copy link
Collaborator

dblock commented Aug 13, 2018

@jnfeinstein Merged, much thanks. You seem to know what you're doing :) Want to help out with the maintenance of mongoid-history? If yes you can make the next release and I'll gladly add you to rubygems (what's your email?).

@jnfeinstein
Copy link
Collaborator Author

Thanks. I'm happy to help out when I can.

@dblock
Copy link
Collaborator

dblock commented Aug 13, 2018

Thanks! Care to email me your rubygems.org email?
Accept your invite to this repo in https://github.com/mongoid/mongoid-history/invitations.

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 this pull request may close these issues.

2 participants