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

Make Squeel compatible with Rails 4.1 and Rails 4.2.0.alpha #317

Merged
merged 7 commits into from Jul 15, 2014

Conversation

@bigxiang
Copy link
Contributor

@bigxiang bigxiang commented Jul 4, 2014

Hi @ernie, @radar, @jonatack

I had made a lot of changes and managed to pass all tests on all versions of Ruby 1.9.3+ and Rails 3+. But I think I indeed need your help because I really pushed a large commit, I don't want to mess anything. Would you please take a check, if you have any questions, we can discuss here.

It should solve #301, #305, #307 one time.

Major Changes:

  • Remove machinist, make model generation more robust and simpler
  • Use polyamorous master because it was just updated
  • Brand new Context class, because JoinAssociation changed a lot
  • Add 4.1 and 4.2 folders under adapters
  • Add some ! methods to visitors, so that we can throw some unexpected behaviors from them
  • Split specs into 3.x-4.0, 4.1, 4.2 folders in order to make tests easier
  • Make all specs green from Rails 3.0
  • Pend few specs because of unnecessary behaviors on Rails 4.1 & 4.2 I think

Thank you very much.

- Remove machinist, make model generation more robust and simplier
- Use polyamorous master because it just compated with Rails master
- Brand new Context class, because JoinAssociation changed a lot
- Add 4.1 and 4.2 folders under adapters
- Add some ! methods to visitors, so that we can throw some unexpected behaviors from them
- Split specs into 3.x-4.0, 4.1, 4.2 folders in order to make tests easier
- Make all specs green from Rails 3.0
- Pend few specs because of unnecessary behaviors on Rails 4.1 & 4.2 I think
@ernie
Copy link
Member

@ernie ernie commented Jul 4, 2014

Nice work, @bigxiang! This is obviously a lot of effort. Some initial feedback based on a quick reading:

  1. I do believe the join dependency stuff belongs in Polyamorous, whose domain the join logic should be. So long as it's not a breaking change there, I'd say we want it there. (it can't rely on Squeel being loaded, of course)
  2. I'm not a big fan of the spec changes, especially the move to shared examples. That being said, since I've bowed out of maintaining Squeel, I wouldn't hold this up for a merge based on that. The polyamorous thing though, I do think means we shouldn't merge this just yet.
@bigxiang
Copy link
Contributor Author

@bigxiang bigxiang commented Jul 5, 2014

Hi @ernie,

Thanks for the feedback. It means a lot.

I will try to find a way to move join dependency stuff in Polyamorous. At the beginning, I was afraid that I would make things unclear if I pushed the code to two gems separately, so I chose to focus on one gem.

I agree with what you said about spec changes. I want to move all split specs into the original file and add some logic based on Rails versions. I just worry about things would be more complicated when Rails keeps upgrading. Should we just let one version of Squeel focus on one version of Rails in the future?

@vendethiel
Copy link

@vendethiel vendethiel commented Jul 5, 2014

Great job ! 👍

what changed based on Rails versions in specs.
- Also remove some dup methods
@bigxiang
Copy link
Contributor Author

@bigxiang bigxiang commented Jul 6, 2014

Hi @ernie , I removed all shared examples in Squeel. it should be easy to review the specs now.

I forgot to mention that Machinist broke the testing with the lasted version of Rails, and it also wasn't maintained, I hadn't any knowledge about it, when I removed it, things went well, so I removed it. How do you think about it? Does it too arbitrary?

@ernie
Copy link
Member

@ernie ernie commented Jul 6, 2014

I think that's fine. I hadn't been aware that Machinist had gone unmaintained.

@ernie
Copy link
Member

@ernie ernie commented Jul 8, 2014

Just a note: I'd like to merge this once we can get the join dependency stuff into polyamorous and out of this patch.

@gernotkogler
Copy link

@gernotkogler gernotkogler commented Jul 8, 2014

I'm happy that Squeel seems to have a future since it is an integral part of our application and we are stuck with rails 4.0.x right now because of squeel. I'd like to help to get it to rails 4.1.x or even 4.2, but my knowledge concerning arel is bare to none. Any ideas how I could help out?

@bigxiang
Copy link
Contributor Author

@bigxiang bigxiang commented Jul 9, 2014

I have removed the code which had been moved to Polyamorous :)

@ernie
Copy link
Member

@ernie ernie commented Jul 9, 2014

@bigxiang I'm still seeing a whole lot of join dependency extensions, unless I'm seeing wrong? Those are all new, and Squeel has never needed them before, especially for 3.x versions. They seem like candidates for Polyamorous to me. Unless @radar or @jonatack see something I don't -- again, I haven't touched the stuff in a while.

@bigxiang
Copy link
Contributor Author

@bigxiang bigxiang commented Jul 10, 2014

@ernie You saw correctly. Because after Rails 4.1, JoinDependency changed a lot, there are new walk_tree and join_constraints methods. I think I can move join_contraints into Polyamorous, but walk_tree is using some classes in Squeel. If I move it to Polyamorous, I have to use some string as conditions instead of class itself, like this:

def walk_tree_with_polymorphism(associations, hash)
  case associations.class.name
    when 'Squeel::Nodes::Stub'   # we don't know Squeel::Nodes::Stub in Polyamorous
      hash[associations.symbol] ||= {}
    when 'Squeel::Nodes::Join'
      hash[associations._join] ||= {}
    when 'Squeel::Nodes::KeyPath'
      walk_through_path(associations.path.dup, hash)
    when 'Hash'
      associations.each do |k, v|
        cache = 
          case k.class.name
          when  'Squeel::Nodes::Stub',  'Squeel::Nodes::Join', 'Squeel::Nodes::KeyPath'
            walk_tree(k, hash)
          else
            hash[k] ||= {}
          end

        walk_tree(v, cache)
      end
    else
      walk_tree_without_squeel(associations, hash)
    end
end

Do you have better suggestions? @radar @jonatack

@ernie
Copy link
Member

@ernie ernie commented Jul 10, 2014

Ugh. I see what you mean. This is just one of those side-effects I've mentioned to you before about how we are digging into stuff that Rails means to be private, and doesn't make easy to extend. :(

@bigxiang
Copy link
Contributor Author

@bigxiang bigxiang commented Jul 10, 2014

I have an idea, it maybe worth trying.

  1. add a module named TreeNode in Polyamorous and give it an unimplemented add_to_tree method
  2. overload walk_tree in Polyamorous to handle TreeNode
  3. let Squeel::Nodes::XXX include TreeNode and implement add_to_tree
  4. remove all join dependency stuff

How do you think about it?

@jonatack
Copy link
Member

@jonatack jonatack commented Jul 10, 2014

Guys, this is interesting stuff and I wish I wasn't swamped right now to join in helping upgrade everything to Rails 4.2 because that's what I'm using (with a slightly-patched version of Ransack that meets my fairly simple needs). @bigxiang you seem up-to-date on the recent changes in AR private methods, more than I am at the moment. Great job. I regret not being more helpful on this right now.

@bigxiang
Copy link
Contributor Author

@bigxiang bigxiang commented Jul 11, 2014

It seems that all new added join dependency stuff was removed :)

…r Rails 4

Fix #312

Examples:

    before:

    Tag.from{first_article.tags.as(Tag.table_name)}.order{tags.name}.to_a
    # => returns error or []

    after:

    Tag.from{first_article.tags.as(Tag.table_name)}.order{tags.name}.to_a
    # => [<Tag>, <Tag>, <Tag>, ...]
@bigxiang
Copy link
Contributor Author

@bigxiang bigxiang commented Jul 15, 2014

@ernie when you have time, would you please have a look ? 😄

Example:

    Person.joins(:articles).where(articles: { person_id: Person.first })
    Person.joins(:articles).where(articles: { person_id: Person.all.to_a })
@ernie
Copy link
Member

@ernie ernie commented Jul 15, 2014

Thanks for this, @bigxiang!

ernie added a commit that referenced this pull request Jul 15, 2014
Make Squeel compatible with Rails 4.1 and Rails 4.2.0.alpha
@ernie ernie merged commit 42dceca into activerecord-hackery:master Jul 15, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@ernie
Copy link
Member

@ernie ernie commented Jul 15, 2014

@bigxiang Thanks for doing the PR -- I think you are definitely the guy Squeel's been waiting for, and will be giving you commit access. I'd also like to get you access to release a new version of the gem, if that sounds good to you?

@bigxiang
Copy link
Contributor Author

@bigxiang bigxiang commented Jul 15, 2014

@ernie, thank you very much. It sounds great. I'm proud that I could do something useful for Squeel. I will keep moving. Thanks for trusting again.

@jarinudom

This comment has been minimized.

Copy link

@jarinudom jarinudom commented on Gemfile in d4cba87 Jul 15, 2014

I love you.

@bigxiang bigxiang deleted the bigxiang:ruby-2-rails-edge branch Jul 16, 2014
@@ -0,0 +1,3 @@
--color
--format documentation
--backtrace

This comment has been minimized.

@ekampp

ekampp Jul 16, 2014

Should this file be in version control?

This comment has been minimized.

@bigxiang

bigxiang Jul 16, 2014
Author Contributor

Ah... I didn't notice it. I just want to let the output of rspec more readable. Does it make any troubles?

This comment has been minimized.

@ekampp

ekampp Jul 16, 2014

I don't care much, @ernie?

This comment has been minimized.

@ernie

ernie Jul 16, 2014
Member

It's OK by me, or I wouldn't have merged :D


create_table :payments do |t|

end

This comment has been minimized.

@ekampp

ekampp Jul 16, 2014

Blank table?

This comment has been minimized.

@bigxiang

bigxiang Jul 16, 2014
Author Contributor

Yes, It just represents the relations, doesn't need any columns. If things changes, we can add some.

@jonatack
Copy link
Member

@jonatack jonatack commented Jul 16, 2014

@bigxiang: 100.times { puts '👍' }

@manuelmeurer
Copy link

@manuelmeurer manuelmeurer commented Jul 16, 2014

Awesome, @bigxiang! Thanks for your work on this! Would love to see you push squeel's development forward! 😸

@ernie
Copy link
Member

@ernie ernie commented Jul 16, 2014

@manuelmeurer That was the point of this recent PR exercise -- it was a trial to take over maintainership of Squeel. @bigxiang passed with flying colors. ;)

@bigxiang
Copy link
Contributor Author

@bigxiang bigxiang commented Jul 18, 2014

Thank you everyone , Thank you very much 🌻 🌻 🌻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.