Rails 4 support #79

Merged
merged 26 commits into from Jul 3, 2013

Conversation

Projects
None yet
7 participants
@spectator
Contributor

spectator commented Feb 15, 2013

Hi Dennis,

Highlight of changes

  • Jeweler replaced with regular gemspec. Sorry for breaking your workflow, however Jeweler stands in the way more than helping. Good thing that rubygems + bundler can do everything you used to with few commands.
  • Conditional Gemfile to easily switch AR versions (3.1, 3.2, and 4.0).
  • Updated AR syntax to Rails 3 and Rails 4 preferred syntax.
  • Updated FactoryGirl.
  • Updated database.yml to avoid some issues with merging keys.
  • Fixed few issues for Rails 4 and updated tests.

There were more changes, commit messages have some more details.

Running tests

RAILS_VERSION=4.0 bundle update
RAILS_VERSION=4.0 bundle exec rake test:sqlite3

Outstanding issues

Rails 4 now does SET SQL_MODE='STRICT_ALL_TABLES' by default which is messing up with the following test

#<Class:0x007f87df0033e0>#test: #import with :synchronization option synchronizes passed in ActiveRecord model instances with the data just imported:
ActiveRecord::StatementInvalid: Mysql2::Error: Field 'title' doesn't have a default value: INSERT INTO `topics` (`id`,`author_name`,`created_on`,`created_at`,`updated_on`,`updated_at`) VALUES (100,'Jerry Carter','2013-06-15 01:06:01','2013-06-15 01:06:01','2013-06-15 01:06:01','2013-06-15 01:06:01'),(101,'Chad Fowler','2013-06-15 01:06:01','2013-06-15 01:06:01','2013-06-15 01:06:01','2013-06-15 01:06:01') ON DUPLICATE KEY UPDATE `topics`.`author_name`=VALUES(`author_name`),`topics`.`updated_on`=VALUES(`updated_on`),`topics`.`updated_at`=VALUES(`updated_at`)

setting strict mode to false (the way all Rails versions prior Rails 4 work) helps to avoid this issue.

I'm not sure how to deal with it, MySQL is clearly complaining on field title that does not have default value in database.

/cc @zdennis

@zdennis

This comment has been minimized.

Show comment Hide comment
@zdennis

zdennis Feb 16, 2013

Owner

Thanks for your work on this. I tackled a bunch of issues this morning and won't have time to investigate your work probably until next weekend. Just wanted you to know that this is on my docket.

Owner

zdennis commented Feb 16, 2013

Thanks for your work on this. I tackled a bunch of issues this morning and won't have time to investigate your work probably until next weekend. Just wanted you to know that this is on my docket.

@spectator

This comment has been minimized.

Show comment Hide comment
@spectator

spectator Feb 16, 2013

Contributor

Thanks. Please let me know if you have any ideas so I can keep digging into issue mentioned.

Sent from my iPhone

On Feb 16, 2013, at 10:56 AM, Zach Dennis notifications@github.com wrote:

Thanks for your work on this. I tackled a bunch of issues this morning and won't have time to investigate your work probably until next weekend. Just wanted you to know that this is on my docket.


Reply to this email directly or view it on GitHub.

Contributor

spectator commented Feb 16, 2013

Thanks. Please let me know if you have any ideas so I can keep digging into issue mentioned.

Sent from my iPhone

On Feb 16, 2013, at 10:56 AM, Zach Dennis notifications@github.com wrote:

Thanks for your work on this. I tackled a bunch of issues this morning and won't have time to investigate your work probably until next weekend. Just wanted you to know that this is on my docket.


Reply to this email directly or view it on GitHub.

@zolzaya

This comment has been minimized.

Show comment Hide comment
@zolzaya

zolzaya Apr 3, 2013

👍

zolzaya commented Apr 3, 2013

👍

@zolzaya

This comment has been minimized.

Show comment Hide comment
@zolzaya

zolzaya Apr 23, 2013

👍

zolzaya commented Apr 23, 2013

👍

@zolzaya

This comment has been minimized.

Show comment Hide comment
@zolzaya

zolzaya Apr 23, 2013

I REALLY need this. Please pull!

zolzaya commented Apr 23, 2013

I REALLY need this. Please pull!

@zdennis

This comment has been minimized.

Show comment Hide comment
@zdennis

zdennis Apr 23, 2013

Owner

If someone can update this pull request so that it can be cleanly merged it will be merged very soon.

Owner

zdennis commented Apr 23, 2013

If someone can update this pull request so that it can be cleanly merged it will be merged very soon.

@spectator

This comment has been minimized.

Show comment Hide comment
@spectator

spectator Apr 23, 2013

Contributor

It's updated, so it can be merged with no issues. However, there is still few unresolved issues. See original message.

Contributor

spectator commented Apr 23, 2013

It's updated, so it can be merged with no issues. However, there is still few unresolved issues. See original message.

@zolzaya

This comment has been minimized.

Show comment Hide comment
@zolzaya

zolzaya Apr 25, 2013

Any news on this issue?

zolzaya commented Apr 25, 2013

Any news on this issue?

@glongman

This comment has been minimized.

Show comment Hide comment
@glongman

glongman Apr 30, 2013

rc1 is out

rc1 is out

@csouls

This comment has been minimized.

Show comment Hide comment
@csouls

csouls May 1, 2013

👍

csouls commented May 1, 2013

👍

@zolzaya

This comment has been minimized.

Show comment Hide comment
@zolzaya

zolzaya May 3, 2013

Any news?

zolzaya commented May 3, 2013

Any news?

@zdennis

This comment has been minimized.

Show comment Hide comment
@zdennis

zdennis May 6, 2013

Owner

I started reviewing this yesterday. I'm going to continue doing this tomorrow night. Tonight, I won't be able to.

Owner

zdennis commented May 6, 2013

I started reviewing this yesterday. I'm going to continue doing this tomorrow night. Tonight, I won't be able to.

@msp

This comment has been minimized.

Show comment Hide comment
@msp

msp Jun 13, 2013

Thanks for your efforts guys. I'd like to use this. Any chance of merging?

msp commented Jun 13, 2013

Thanks for your efforts guys. I'd like to use this. Any chance of merging?

@spectator

This comment has been minimized.

Show comment Hide comment
@spectator

spectator Jun 14, 2013

Contributor

I finally did another look at the remaining issue and it turns out that Rails 4 has strict mode enabled for mysql. See rails/rails#6069 and rails/rails#6069

I'm not very familiar with that so I will need to look more into to figure out

  • if it's mysql feature/issue to fail if column is not listed even though it has value
  • to make rails app work with strict mode, but ar-import in traditional mode

Whoever needs this asap can try using my branch and report if there is any issues. Do not forget to update your database.yml as I did here f735477

Contributor

spectator commented Jun 14, 2013

I finally did another look at the remaining issue and it turns out that Rails 4 has strict mode enabled for mysql. See rails/rails#6069 and rails/rails#6069

I'm not very familiar with that so I will need to look more into to figure out

  • if it's mysql feature/issue to fail if column is not listed even though it has value
  • to make rails app work with strict mode, but ar-import in traditional mode

Whoever needs this asap can try using my branch and report if there is any issues. Do not forget to update your database.yml as I did here f735477

@msp

This comment has been minimized.

Show comment Hide comment
@msp

msp Jun 16, 2013

Thanks for the update @spectator. For now I'll just loop in a few of my team mates and we'll prioritise this with our Rails 4 upgrade path cc @dandan @andistuder

Hopefully we can contribute.

msp commented Jun 16, 2013

Thanks for the update @spectator. For now I'll just loop in a few of my team mates and we'll prioritise this with our Rails 4 upgrade path cc @dandan @andistuder

Hopefully we can contribute.

@jcoyne

This comment has been minimized.

Show comment Hide comment
@jcoyne

jcoyne Jul 2, 2013

👍 Blocked by this.

jcoyne commented Jul 2, 2013

👍 Blocked by this.

+
+version = ENV['RAILS_VERSION'] || "3.2"
+
+eval_gemfile File.expand_path("../gemfiles/#{version}.gemfile", __FILE__)

This comment has been minimized.

Show comment Hide comment
@zdennis

zdennis Jul 3, 2013

Owner

I really like how you're loading gemfiles for different versions of Rails.

@zdennis

zdennis Jul 3, 2013

Owner

I really like how you're loading gemfiles for different versions of Rails.

This comment has been minimized.

Show comment Hide comment
@spectator

spectator Jul 3, 2013

Contributor

Yeah, I wasn't sure whichever is better. AR_VERSION sounds good.

@spectator

spectator Jul 3, 2013

Contributor

Yeah, I wasn't sure whichever is better. AR_VERSION sounds good.

This comment has been minimized.

Show comment Hide comment
@spectator

spectator Jul 3, 2013

Contributor

Ouch, replied to wrong message.

Thanks, I think Steve Klabnik blogged about this so this is his idea :)

@spectator

spectator Jul 3, 2013

Contributor

Ouch, replied to wrong message.

Thanks, I think Steve Klabnik blogged about this so this is his idea :)

+ gem "debugger"
+end
+
+version = ENV['RAILS_VERSION'] || "3.2"

This comment has been minimized.

Show comment Hide comment
@zdennis

zdennis Jul 3, 2013

Owner

RAILS_VERSION is shorter but technically it should be ACTIVERECORD_VERSION or AR_VERSION. Thoughts?

@zdennis

zdennis Jul 3, 2013

Owner

RAILS_VERSION is shorter but technically it should be ACTIVERECORD_VERSION or AR_VERSION. Thoughts?

@zdennis zdennis merged commit 161dc35 into zdennis:master Jul 3, 2013

@zdennis

This comment has been minimized.

Show comment Hide comment
@zdennis

zdennis Jul 3, 2013

Owner

@spectator, sorry about the wait and thank you for submitting the pull request. This is all out in master now. I will prepare a release this weekend.

Owner

zdennis commented Jul 3, 2013

@spectator, sorry about the wait and thank you for submitting the pull request. This is all out in master now. I will prepare a release this weekend.

@spectator

This comment has been minimized.

Show comment Hide comment
@spectator

spectator Jul 3, 2013

Contributor

Thank you!

Contributor

spectator commented Jul 3, 2013

Thank you!

@msp

This comment has been minimized.

Show comment Hide comment
@msp

msp Jul 4, 2013

Thanks guys.

msp commented Jul 4, 2013

Thanks guys.

@zdennis

This comment has been minimized.

Show comment Hide comment
@zdennis

zdennis Jul 17, 2013

Owner

This has been released tonight with activerecord-import 0.4.0.

Owner

zdennis commented Jul 17, 2013

This has been released tonight with activerecord-import 0.4.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment