search

jackdempsey / acts_as_commentable

The ActiveRecord acts_as_commentable plugin
http://juixe.com/svn/acts_as_commentable/
MIT License
834 stars 176 forks source link

Requesting Update to Gem on RubyGems.org #23

Closed jrreed closed 11 years ago

jrreed commented 12 years ago

Hi,

I was wondering if you guys could cut a new gem containing the changes that @mickey added back in 12/2010 to support multiple kinds of comments in a single model as well as the supporting methods and tests? It looks like a very useful piece of functionality.

Cheers, James

mickey commented 12 years ago

FYI we're using our fork in production for about 2 years but I can't assure you it wont break anything; we only use on one application and didn't touch this feature since then. Mention to @mrzor that recently added rails 3.2 compat' on the rails 3.2 branch.

jackdempsey commented 12 years ago

I'd be happy to update things--will need some assistance though. @jrreed if you want to throw over a pull request with the latest from @mickey and citizencast, I'll merge and update the Gem. I just don't currently have the time to put this all together, make sure nothing's broken, and wouldn't want to update a well known gem like this without strong confidence all new code is good. Thx!

jrreed commented 12 years ago

Sorry, at the time, I hadn't realize @mickey's functionality added a breaking change for rails 3.2. I thought it was just a matter of cutting a new gem.

I'd be happy to put together a pull request, but I'm not sure how to resolve the issue of supporting both write_inheritable_attribute (which was removed in 3.2) and class_attribute (which wasn't available until 3.0).

I'm still new to Rails, and would be very interested in learning how to resolve an issue like this. If there's a blog post, another gem that makes a good example of how to handle this, I'd be more than willing to take a stab at it.

I'm also ok, with just forking and making changes locally, and closing this issue. Either way works for me.

jackdempsey commented 12 years ago

hey @jrreed, thanks for the thoughts, and welcome to Rails!

Here's my suggestion: fork the change and get it to work locally. We'll keep the issue open and if you and/or others can help with it, great.

There's no easy way to handle the differences in versions you mentioned--one thought I'm having though is that we bump the gem to 4.0, make sure it works against Rails master (targeting 4.0), and then don't worry about past compatibility.

Anyone who bundled against AAC < 4 should be fine, and if you haven't specified a version and update your gems and things break...not much we can do about that.

mickey commented 12 years ago

I forked the plugin and created a 4.0 branch.

I included our company fixes and added a few more fixes to make it work on rails master. Tests are green but there's not a lot of them.

jackdempsey commented 12 years ago

hey @mickey , thanks! I've finally had a chance to look over the changes. I've merged them to my repo and pushed back to github. Couple questions:

  1. Is pry really necessary? I'd prefer to keep that requirement out, but didn't want to remove it as I'm not sure what's depending on it.
  2. The instructions for rails 3 seem wrong: 2.0.1? Not 3.0.1? Double checking before I change.
  3. A ruby 1.9 dependency kinda snuck in there, right? (The use of -> ). Do you want to add that note to the README?

thanks!

PikachuEXE commented 11 years ago

@jackdempsey Your repo is missing a tag for 3.0.1 :3 And ya for rails 3 it should be 3.0.1

jackdempsey commented 11 years ago

thx for reminder. 3.0.1 tag is up, couple other tweaks, and a 4.0 gem is pushed to rubygems.

On Sat, Sep 29, 2012 at 7:17 PM, PikachuEXE notifications@github.comwrote:

@jackdempsey https://github.com/jackdempsey Your repo is missing a tag for 3.0.1 :3 And ya for rails 3 it should be 3.0.1

— Reply to this email directly or view it on GitHubhttps://github.com/jackdempsey/acts_as_commentable/issues/23#issuecomment-9010126.