The Version class isn't namespaced

[hoverbird]

This means it doesn't play well with RailsAdmin, even though RailsAdmin's Version class IS namespaced: railsadminteam/rails_admin#1234

[hoverbird]

I recognize this will be a breaking change, depending on how folks use PaperTrail... so this is to get a conversation going as much as anything else.

[zolzaya]

+1 for namespacing Version class.

[hoverbird]

Bump... any changes I could make to get this shippable? Maybe in the next major release, if one is planned?

[batter]

@hoverbird - I'm not totally against the idea of namespacing the version class for PaperTrail, but I do feel that it makes it a little more painful to work with in the sense that it requires you to prepend PaperTrail:: for every call to Version.

Im also concerned because as you mention above:

I recognize this will be a breaking change, depending on how folks use PaperTrail... so this is to get a conversation going as much as anything else.

What is the purpose of RailsAdmin having a class specifically for the purposes of Version? I wonder about (the change resulted from this commit). I just poked around github and looked at some of the more prominent gems available, and noticed that none of them have their own class dedicated to the purpose of providing a version. So what is the purpose of this? Even the rails gem itself keeps the Version information namespaced within a module. The jewler gem has a section of their documentation on versioning where it shows an example of making the Version into it's own module, but not a class.

So I am curious as to why @sferik has chosen to make a class entirely for the purpose of conveying the version of the RailsAdmin gem.

[sferik]

The fact that I decided to define RailsAdmin's version in a class is not the issue. I'm not going to engage in an argument about whether this is good or bad OO design. Let's just say you're right; this is a bad design.

If you aim to create a library that works with unspecified other libraries, you need to namespace all of your classes. You can't just define a Version class in the global namespace and expect every other library author in the Ruby ecosystem to telepathically avoid defining a class of the same name. This is precisely the reason why namespaces exist.

I have very little sympathy for the argument that namespacing your Version class is bad because it requires extra typing. If you want to dump everything in the global namespace, perhaps you'd be more comfortable in the PHP community, circa 2008.

[batter]

@sferik I wasn't trying to declare that it was bad design, I was merely curious as to what led you to choose that implementation. My apologies if I came across as brash.

I agree with you that extra typing is not a good reason refrain from namespacing the Version class. I do agree that there are merits to doing this.

The reason I don't automatically assume that this makes sense, is because this gem is specifically made as an add-on to rails. Out of the box, model classes don't come namespaced, and thus this is why it makes me a little uneasy to suddenly namespace it with no announcement.

And as mentioned by @hoverbird, Existing users who upgrade to a version of the gem which contains this modification will need to change some of their configuration in order for it to work without breaking as well. I will have to give this a bit more consideration and discuss it with the original author of the gem, @airblade. It seems to me that a change like this could go in conjunction with removing the dependency on Sinatra (#119) and might even warrant a new major release.

[airblade]

On the one hand, as @fullbridge-batkins says, plain ActiveRecord classes aren't normally namespaced. On the other hand, as @sferik says, "version" is a pretty common name.

I'm not averse to moving PaperTrail's Version class into a paper_trail module. However because it would not be backwards compatible, it would need to happen as part of a major release as mentioned by @fullbridge-batkins.

[hoverbird]

@airblade while ActiveRecord classes are not by convention namespaced, this tends to be OK because the first party is writing all their own classes and can manage name conflicts within their project/organization. This breaks down when you depend on libraries from multiple 3rd-party vendors... you don't know when they'll introduce a class named like one of your or another party's (note @jeremedia's collision, which is not with another gem altogether, not RailsAdmin). Say what you will about Rails not going the Java route of namespacing all the things, even first-party code, but I think it's clear that rails_admin is a better citizen for keeping the global namespace tidy.

Maybe there's a way to make the transition to the namespaced Version easier... the combination of a warning for those who reference the global, now-missing class, with a configuration option to dump it back into the global namespace for those unable to upgrade? I'm just spitballing, not sure quite how I'd rig this up. Maybe @sferik has some ideas?

[sferik]

Moving the Version class into the PaperTrail module will necessarily be a breaking change and require a major version bump but there are a few things you can do now to ease the transition. The first is supporting both the namespaced and non-namespaced Version class in current releases and encouraging users via a gem post-install message and deprecation warnings.

I'm not at all familiar with the paper_trail codebase, so this might not actually work, but in theory you should be able to provide both a namespaced and non-namespaced Version class via subclassing:

module PaperTrail
  class Version
    # Put all the code in the Version class here
  end
end

class Version < PaperTrail::Version
  def initialize
    warn "DEPRECATED: Please use the namespaced PaperTrail::Version class instead."
    super
  end
  # Put any code required to maintain backward compatibility here
end

[hattwj]

+1 for sferik's idea. I was just about to say the same thing and found out that he already said it. Also just wanted to say hi. I think I will be using PaperTrail on a few projects.

[batter]

Agreed, this does seem like the best approach for the implementation. I've been awfully busy with my day job in the last couple weeks but I'm getting near to the point where I can start tackling some of the issues on the roadmap for 3.0.