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

Move rails 4.0 compatibility support into its own files like the other versions #416

Closed
avit opened this issue Aug 15, 2014 · 11 comments
Closed
Assignees

Comments

@avit
Copy link
Contributor

avit commented Aug 15, 2014

I'd like to clean up the adapters directory to remove the version conditionals from the latest version. Does it make sense to move 4.0 compatibility out to a separate file like the others now? It's getting messy.

It could end up like the 3.x series:

adapters/active_record/context.rb for latest 4.x requires 4.0/context.rb

Another thought was to move everything into feature modules (by version) and just use the main context.rb adapter file to switch the inclusion of the right feature modules. Any thoughts?

@jonatack
Copy link
Contributor

I agree, good idea.

What avantage would the feature modules by version have over the current implementation in Ransack::Adapters::ActiveRecord::ActiveRecord?

@avit
Copy link
Contributor Author

avit commented Aug 15, 2014

I think maybe less duplication between the different files. Right now It's hard to see at a glance what's where, and what gets overridden. I'll think on it and see what's worth doing.

@avit
Copy link
Contributor Author

avit commented Sep 17, 2014

@jonatack What's your branch maintenance strategy so I can design around that?

I assume the ransack gem is built from master, so the other branches are just there for anyone who wants to eliminate the older version conditionals & get some speed boost from the latest code, at least according to the README.

Do you intend to maintain the different rails versions as standalone branches? (Challenge: shared enhancements need to propagate out.)

Or do the different version branches eventually get merged into master? (Challenge: needs to support merging all versions in one codebase.)

I'm leaning towards a single branch for maintenance. If we refactor the way we require adapters & extract some internal helper methods to encapsulate the external version differences, we could still keep the core codebase free of version conditionals.

@jonatack
Copy link
Contributor

Your assumption is exactly correct.

The other branches were each born when I needed to make Ransack experimentally compatible with Rails master at that time. Maintaining them does add to the workload, and they are often a bit behind the master branch while we try new PRs out on master, before propagating them out to the branches.

Once Rails 4.2 is released and master moves to 5.0, and the next Ransack gem is released with full compat from 3.0 to 4.2, I'll create a Ransack rails-5.0 branch and plan to stop actively maintaining the other branches and let the community do it if they are interested.

@jonatack
Copy link
Contributor

Your idea to have a single branch for maintenance and refactor out the version differences would be awesome 😃

@avit
Copy link
Contributor Author

avit commented Sep 17, 2014

Great! I'll put this on my TODO pile and try to get to it soon-ish.

@jonatack
Copy link
Contributor

jonatack commented Nov 7, 2014

After the work done by @Zhomart for the Mongoid adapter, splitting things out, it seems like a good time to clean up the Active Record adapter, if you have time?

@jonatack jonatack self-assigned this Aug 18, 2015
@avit
Copy link
Contributor Author

avit commented Feb 17, 2016

Oh my goodness I so want to dive in and do this after my recent adventures in this codebase again...

  • Always load one base file, then load version compatibility files as overload
  • Move Arelish/Visitory things out of Nodes classes (& somewhat from Context)
  • Make different Visitors for different things instead of conditionals
  • Restore sanity

Not sure when I'll ever have the time, but I might just make smaller attempts at this as I pick off other fixes with my drive-by pull requests.

@scarroll32
Copy link
Member

@avit would be great to have your contributions ... any progress on this?

@scarroll32
Copy link
Member

If we can wait for EOF on Rails 4.2, I vote for removing the code from older versions completely, or leaving available on a branch but removed from master.

https://stackoverflow.com/questions/43050213/ruby-on-rails-4-2-end-of-life

@scarroll32
Copy link
Member

Looks like this has been done. Issue closed.

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

No branches or pull requests

3 participants