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

Update for Rails 4.2.3 #1

Merged
merged 4 commits into from
Jul 1, 2015
Merged

Update for Rails 4.2.3 #1

merged 4 commits into from
Jul 1, 2015

Conversation

petelouise
Copy link

Allows newer versions of ActiveRecord.
Fixes make_flaggable_spec so that it will run, but does not fix make_flaggable_generator_spec since it requires a change in the generator_spec gem.

@@ -1,14 +1,17 @@
ActiveRecord::Schema.define :version => 0 do
create_table :flaggable_models, :force => true do |t|
t.string :name
t.integer :flaggings_count
Copy link

Choose a reason for hiding this comment

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

What did you do in order to make these changes? Was it just running a db:migrate?

Copy link
Author

Choose a reason for hiding this comment

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

I added them by hand. They would have become necessary after the counter cache was added, but no one updated the specs.

@jhanggi
Copy link

jhanggi commented Jul 1, 2015

Was make_flaggable_generator_spec broken before the upgrade? What sort of change does it need?

@petelouise
Copy link
Author

Yes, all of the specs were broken before the upgrade. Even before I made any changes to the repo, none of the specs would run. Some of them were due to config problems, and then the test schema didn't have the right columns.

There is a custom matcher in the generator_spec gem that I think is causing problems, but I didn't spend a lot of time looking into it. The only spec that's failing is one about the generator creating files in the correct places. Ideally, the spec and generator should be updated with the columns needed for counter_cache. But since we're not using the generator in GS, I figured it didn't make sense to fix up everything.

@@ -14,8 +14,8 @@ Gem::Specification.new do |s|
s.required_rubygems_version = ">= 1.3.6"
s.rubyforge_project = "make_flaggable"

s.add_dependency "activerecord", ['>= 3.0', '< 4.2']
s.add_development_dependency "bundler", ['>= 1.0.0', '<= 1.4']
s.add_dependency "activerecord", '< 4.3'
Copy link

Choose a reason for hiding this comment

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

I think we should maintain the >= 3.0 because I do believe it relies on syntax from AR 3. And we should issue a PR against the original with all these changes. I'd be comfortable we just said >= 3.0 without the any upper limit.

@petelouise
Copy link
Author

Updated:

  • Change ActiveRecord version to >= 3.0
  • Add .rb to generated migration file

@jhanggi
Copy link

jhanggi commented Jul 1, 2015

👍 Can you rebase this PR into a couple discrete commits that explain the steps?

So removing the duplicate gem makes sense as a commit, as does adding the .rb (reference medihack#26) in that commit, and then the Rails other requirements into a commit so there isn't a back and forth. I trust your judgement to tell a good story.

@jhanggi
Copy link

jhanggi commented Jul 1, 2015

And then once you're done with that and have this merged in, issue a PR against https://github.com/medihack/make_flaggable

@petelouise petelouise force-pushed the update_for_rails_4_2_3 branch from a8bdc6f to f00795b Compare July 1, 2015 18:16
petelouise pushed a commit that referenced this pull request Jul 1, 2015
@petelouise petelouise merged commit a159232 into master Jul 1, 2015
@petelouise petelouise deleted the update_for_rails_4_2_3 branch July 1, 2015 18:24
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