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

Updates to support Rails4 #199

Merged
merged 4 commits into from
Aug 1, 2013
Merged

Conversation

chrisnicola
Copy link
Contributor

This will only support Rails4 as I'm not sure if you prefer to keep
a separate semantic version for Rails3 instead of trying to support both
in a single gem.

It should be fairly straightforward to support both, though testing
for each major version of rails could be quite tedious.

@ghost ghost assigned batter Feb 5, 2013
@batter
Copy link
Collaborator

batter commented Feb 5, 2013

Thanks for the pull request. In a week or two, after 2.7.1 is released, I'm going to branch the repository and begin work on 3.0.0 . I'm hoping to tackle some bigger picture issues for 3.0.0, this could be a prime candidate, although we may look into removing the dependency on rails altogether (the idea being that the gem could be used in other frameworks such as sinatra). I'll take a good look at this around that time and if nothing else I'll start preparing a branch that is compatible with Rails 4. In the meantime, any suggestions and patches you provide are much appreciated.

@reggieb
Copy link

reggieb commented Feb 7, 2013

+ 1 for removing rails dependency. 

I love rails, but minimising a gems dependency is always a good idea.

@parndt
Copy link
Contributor

parndt commented Feb 14, 2013

@fullbridge-batkins can you please open a branch now and merge this so that other libraries (a la globalize3) can work on integration support now? Thanks!

@woto
Copy link

woto commented Feb 15, 2013

+1 works good nice to see it in master branch.
Sorry for bad Englis.

@batter
Copy link
Collaborator

batter commented Feb 15, 2013

@parndt - Done. Branch is named rails4.

@iain
Copy link

iain commented Feb 26, 2013

There is a deprecation warning in Rails4 that this PR doesn't fix yet. The has_many that has_paper_trail adds, uses a scope without a lambda. I don't know how to contribute to someone else's PR though....

@parndt
Copy link
Contributor

parndt commented Feb 26, 2013

When you open a PR you can choose the base repo to be the PR owner and the base branch to be the PR branch so in this case you'd use lucisferre repo and rails4 branch. Locally you just use git fetch:

git remote add -f lucisferre git://github.com/lucisferre/paper_trail
git checkout lucisferre_rails4_patch --track lucisferre/rails4
# make changes and add, make a commit
git push origin lucisferre_rails4_patch

Now, on GitHub, open the pull request against lucisferre repo and rails4 branch.

Hope that helps and that my instructions are correct.

@leckylao
Copy link

@iain confirmed the deprecation warning, using branch: rails 4

Loading development environment (Rails 4.0.0.beta1)
irb(main):001:0> Event
DEPRECATION WARNING: The following options in your Event.has_many :versions declaration are deprecated: :order. Please use a scope block instead. For example, the following:

has_many :spam_comments, conditions: { spam: true }, class_name: 'Comment'

should be rewritten as the following:

has_many :spam_comments, -> { where spam: true }, class_name: 'Comment'

. (called from has_paper_trail at /home/leckylao/.rbenv/versions/2.0.0-rc2/lib/ruby/gems/2.0.0/bundler/gems/paper_trail-283f13139ac0/lib/paper_trail/has_paper_trail.rb:60)
=> Event(id: integer, name: string, description: string, user_id: integer, team_id: integer, datasource_id: integer, eventclass_id: integer, client_id: integer, updated_by: integer, created_by: integer, status: string, priority: integer, target_date: date, created_at: datetime, updated_at: datetime)

@mathias
Copy link

mathias commented Mar 8, 2013

Thank you for the work on this! Looking forward to using paper_trail with my Ruby 2.0 + Rails 4 app 👍

@batter
Copy link
Collaborator

batter commented Mar 15, 2013

@iain, @lecky, I just pushed 009d3bd to the rails4 branch, which should get rid of those pesky deprecation warnings.

@leckylao
Copy link

Good! Thank you. Glad to hear that.

Sent from my iPhone

On 16/03/2013, at 4:21 AM, Ben Atkins notifications@github.com wrote:

@iain, @lecky, I just pushed 009d3bd to the rails4 branch, which should get rid of those pesky deprecation warnings.


Reply to this email directly or view it on GitHub.

@mat813
Copy link
Contributor

mat813 commented Jul 1, 2013

Hello there,
Rails 4 is out now, do we have a timeline on a new release ?

@batter
Copy link
Collaborator

batter commented Jul 2, 2013

Trying to find some extra time to work on some of the outstanding pull requests. The master branch is now the working copy that will become version 3.0.0 eventually, I'm leaning towards making that a rails4 exclusive branch, but may even try to remove the dependency on rails entirely, as has been discussed.

For the time being you can continue using the rails4 branch since it contains all of the features from the officially released 2.7.2 branch.

@batter
Copy link
Collaborator

batter commented Jul 31, 2013

@chrisnicola - I'm making an effort to merge this pull request into the master branch, but I have a question. Is there a reason why you swapped in MiniTest for Shoulda? Maybe I just don't know the proper syntax for MiniTest, however, running rake test with your changes seems to result in a lot of tests (anything nested inside of a describe block really) not being executed unless I'm mistaken.

@batter batter merged commit 283f131 into paper-trail-gem:master Aug 1, 2013
@chrisnicola
Copy link
Contributor Author

You know what, I really should have written more detailed commit messages, sorry. It's been 6 months and I can't recall exactly, I think it might have had something to do with Rails 4 switching to MiniTest as the default test framework used by ActiveSupport::TestCase and Shoulda wasn't working properly for me?

Sorry, if it works without those changes feel free to revert the tests.

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.

10 participants