Updated gem to work (and successfully test) with Rails 5.0

[mvastola]

All tests for Rails versions 4.x.y thru 5.0.x now passing. (Previously the test suite had been left unmaintained and only passed with Rails 4.0.x.)
Nearly all warnings/deprecations resolved.

This PR resolves #37 and should provide for full Rails 5.0.x compatibility. I therefore request a new version be released once these changes are merged.

[mvastola]

Update: PR now includes addition of appraisal gem to enable CI testing against multiple possible Ruby/Rails version combinations (Rails versions: 4.0.x, 4.1.x, 4.2.x, 5.0.x; Ruby versions: 1.9, 2.0, 2.1, 2.2.4, 2.3.0, ruby-head).

All compatible Ruby/Rails version pairs have passing tests.

[mvastola]

@rafaelfranca and others, FYI: I just updated this PR to resolve the final deprecation warning, to add a CI test against ruby 2.3.0 (passing with all Rails versions), and to rebase my repo against changes to rails-observers that have been committed since I first submitted this PR.

Is there any particular ETA on this PR being reviewed? I'm eager to see it merged. (Especially since another gem relies on it being merged.)

[rafaelfranca]

Could you remove appraisal? It is not necessary and I prefer to keep our setup simple.

[mvastola]

Happy to do so. Would you like to keep the separate Gemfiles for the different versions of Rails without appraisal though? So you can be sure that future changes don't break compatibility with older versions of Rails without having to manually test each one?

[rafaelfranca]

Yes. We should keep the Gemfiles, just not use the appraisal gem.

[mvastola]

Got it.

[rafaelfranca]

Awesome work! I already reviewed it and commented.

[mvastola]

@rafaelfranca Thanks a ton! I'll make the changes we discussed now.

[mvastola]

@rafaelfranca Ok. Changes made. See the circular dependency errors. Any ideas? Seems the problem started existing with Rails 4.1. (Before I touched them with this PR, the tests had only been configured to work with Rails 4.)

[rafaelfranca]

Ok. Seems like it is trying to load the User constant during it is already loading the User constant. The cause seems to be the line 22 in the railtie. Try to remove it

[mvastola]

@rafaelfranca Ok. That did it. Everything appears to be passing now. Are we all set?

[mvastola]

Thanks!
@rafaelfranca Could I ask you to push a new version?

[rafaelfranca]

Release a new version is not possible now. We need to get master version tested because it may have different behavior now. I'd recommend you to use the master version in your application to see if it works.

[mvastola]

Ok. Well I'm going to put together a PR for audited (a gem which depends on rails-observers) and try its batch of tests against the master branch. I'll let you know how that works.

[mvastola]

@rafaelfranca See collectiveidea/audited#252. All rspec tests for the audited gem are now passing when tested against the master branch of rails-observers on Github after minor changes are made for compatibility with Rails 5. (Note that I don't expect this PR to be merged until a release is made of this gem for obvious reasons.)

Is there anything else you'd like to see before you're comfortable pushing a new version?

[kelhusseiny]

@mvastola 👍 I've upgraded to rails 5, added rails-observers ref to master into Gemfile with your fork there collectiveidea/audited#252 is working perfectly. All tests a green now. Waiting for pushing a new version here.

[rafaelfranca]

Cool! I'll release it tomorrow.

[mvastola]

Thanks so much!

[mvastola]

@rafaelfranca Do you have a new expected date for this?

[mvastola]

Hi, @rafaelfranca, as it's been another week, just another gentle reminder to push a release for this gem..

[mvastola]

@rafaelfranca, just checking in on this again.. it's been nearly a month (it will be exactly one month on Friday) since you said you'd release this.. Is there some sort of complication?
As I'm sure you know, the release of Rails 5 is expected later in February, and this is impeding the adoption of Rails 5 compatibility in another gem.

[jachenry]

@mvastola My observers stopped working after pointing to master. Were there any changes you made that requires special behavior for running observers in rspec? Reverting back to 876c522 resolved the issue.

[mvastola]

@jachenry I'm not sure what you mean by "special behavior for running observers in rspec".. Could you clarify?

You shouldn't need to make any changes to your production code or tests to apply this update.

What version of rails are you on? Can you offer an example app with non-functional observers?

[mvastola]

Also, @jachenry, out of curiosity, try switching back to master and putting the following in an initializer (@rafaelfranca had me remove this line to fix a bug and that was the only potentially substantive change that was really made) and see if it works then:

ActiveSupport.on_load(:active_record) do
  ActiveRecord::Base.instantiate_observers
end

[mvastola]

@jachenry Are you going to be able to provide more information on your problem? This should also really go in it's own issue now that this is merged. (Feel free to tag me)

@rafaelfranca I have not heard from you at all on this.. (to the point that I'm not even sure you're getting these messages). I'm tempted to actually make an issue about the lack of a release itself if I don't hear back from you in the next day or so.

[mvastola]

@rafaelfranca upon further investigation, the CI tests don't seem to test that observers are instantiated. At the very least I want to make a test and ensure that it's passing against the current code before we release. I plan to write the test either tonight or tomorrow. Please still reply though so I know that someone's out there.

[mvastola]

Ok, I just added the test at https://github.com/Partyista/rails-observers and it is failing. I'm going to make a new PR to discuss fixing this issue and then we can talk about a release.

[shlok007]

Can we have the version release please?

[kaspth]

@shlok007 #53 (comment) 😁

[shlok007]

@kaspth Looking forward for the release soon. 👍

[mvastola]

Hey guys,
The real issue here is in #41, so it would probably be best if all comments were redirected there. It would be even better if someone wanted to help work on that PR as my time is limited. (I'm happy to grant access to my fork.)

Long story short:
I introduced a regression in this PR (#39) which, after it was pointed out to me, I promptly fixed with #41 (which, at the time, passed all CI tests and was merge-conflict-free).

Unfortunately, by the time someone else verified the fix in #41, commits had already been made that resulted in merge conflicts between #41 and HEAD. While these conflicts were simple enough to resolve (and have been resolved in #41 as it stands), the CI tests no longer pass (suggesting the commits to master after #39 was merged made some incompatible changes) and thus far, I have not been able to successfully resolve the errors.