Rails Ruby on Rails

Comments and changes to this ticket

• You flagged this item as spam.
  

  Bodaniel Jeanes October 7th, 2010 @ 11:21 PM

  Jon, perhaps prefix the title of this ticket with [PATCH] so they know there's a patch to apply/test instead of a problem to solve.

  FWIW, +1 and this change introduces no extra detectable bugs and will go along way with regards to Jon and I finishing integrated nested has_many :through support in #1152

• 

  Jon Leighton October 7th, 2010 @ 11:22 PM

  • Title changed from “Refactoring JoinDependency and friends” to “[PATCH] Refactoring JoinDependency and friends”
• 

  Jon Leighton October 7th, 2010 @ 11:23 PM

  I thought that the 'patch' tag was the official way to do it, but if [PATCH] in the title helps then why not :)

• 

  Ryan Bigg October 8th, 2010 @ 03:01 AM

  • Importance changed from “” to “Low”

  +1 from me too. This patch applies cleanly and the tests run.

• 

  Ryan Bigg October 8th, 2010 @ 03:04 AM

  • Importance changed from “Low” to “Medium”

• You flagged this item as spam.
  

  Alex MacCaw October 8th, 2010 @ 02:57 PM

  +1

• 

  Jon Leighton October 8th, 2010 @ 03:02 PM

  • Tag changed from “activerecord, associations, patch” to “activerecord, associations, patch, verified”
• 

  Jon Leighton October 8th, 2010 @ 03:05 PM

  • Assigned user set to “Santiago Pastorino”

  Assigning to Santiago as that's what it says to do here: https://rails.lighthouseapp.com/projects/8994/ticket-assignment

• 

  Santiago Pastorino October 8th, 2010 @ 06:29 PM

  • Assigned user changed from “Santiago Pastorino” to “Aaron Patterson”
• 

  Aaron Patterson October 11th, 2010 @ 06:27 PM

  • State changed from “new” to “needs-more-info”

  Hi @Jon,

  It's very difficult for me to review this patch. Could you break it up in to multiple patches please? It looks like you're renaming variables and changing formatting. It's difficult for me to tell whether you're just renaming and reformatting, or actually changing stuff.

  Would you mind breaking this up to one patch that does formatting and renaming changes, and one patch that makes any refactors (like new methods, etc)?

  Thanks.

• 

  Jon Leighton October 12th, 2010 @ 11:05 AM

  Hi Aaron,

  Thanks for taking time to look at this, and sorry that my patch confused you.

  I'm happy to split it up if that helps, but I thought perhaps it might be more useful if I provided a better explanation of what the patch actually changes. (Admittedly I should have done this originally.)

  • Currently JoinAssociation has the methods #relation, #association_join and #join_class. These are used together in ActiveRecord::QueryMethods.build_joins to build up the joins which are necessary to join an association onto a query.
  • The current situation is that joining an association will either result in one or two actual SQL JOIN statements being generated (depending on whether the association in question involves a join table or not). The way that JoinAssociation deals with this is to have #relation either output a single Arel::Table for a single join, or to output an array containing two Arel::Tables, for two joins. In the latter case, #association_join also outputs an array which has the different conditions for different joins at different positions in the array.
  • The work I am doing on having arbitrarily nested associations (x goes through y which in turn goes through z) means that the current JoinAssociation is inflexible for this, because it is geared towards only having either one or two joins. Additionally the code already uses special cases in different situations to achieve its ends (for example, testing whether the return value of association.relation is an array or not, in build_joins).
  • I wanted to refactor how JoinAssociation and build_joins worked to make them more generic.

  This is the basic rationale. In terms of what I actually did:

  • As I said, currently JoinAssociation has various methods which output various things. build_joins uses their output to produce the actual join, e.g. relation.join(some_table).on(*some_conditions).
  • Instead of this, I let JoinAssociation have a #join_to(relation) method, with the idea that JoinAssociation would now be responsible for adding as many joins to the relation as are necessary. #join_to would then return a relation with the joins added. This prevents the situation of having to pass around arrays where the different positions in the arrays take on a special meaning, etc.
  • As you can see in the patch, this makes the code in build_joins a lot nicer - instead of all that stuff with building up the to_join array, we just iterate each of the JoinAssociations and do relation = association.join_to(relation)

  I also made some slightly more cosmetic changes, which seemed to 'make sense' as part of it:

  • Currently JoinAssociation is a subclass of JoinBase. Instead I created an abstract JoinPart class which JoinAssociation and JoinBase both individually inherit from. This seems cleaner to me, as I don't think a JoinAssociation is a type of JoinBase, though they are related. For example JoinBase has the table_joins attribute which means nothing to JoinAssociation.
  • Due to the addition of this JoinPart superclass, I thought it was more consistent to name the collection in JoinDependency to join_parts instead of just joins
  • I also renamed JoinAssociation#join_class to join_type. This is purely me being opinionated about what it should be called. I'm not attached to that name and it doesn't fundamentally matter either way.
  • I didn't make any sweeping formatting changes or anything like that.
  • I didn't change any of the logic which actually builds the joins and their conditions.

  I hope this explanation helps. I'm willing to break up the patch if you'd still like me too, but obviously happy to avoid that work if possible :) Let me know how you think it's best to proceed.

  Cheers,
  Jon

• 

  Jon Leighton October 12th, 2010 @ 11:06 AM

  Also - if you'd like to have a chat on Skype or anything like that then let me know.

• 

  Aaron Patterson October 12th, 2010 @ 06:59 PM

  Damn! Thanks for the detailed explanation, I really appreciate it. The problem though, is that the first ~200 lines of the patch are mostly formatting and changing variable names. I think there might be one or two lines out of the first 200 that are substantial.

  I'm in favor of fixing formatting and changing variable names, but it doesn't help me understand the core of the patch. I like your description of the changes and I am in favor of adding this functionality, but please split this up.

  I would really like one patch that does variable renaming and formatting changes, and one that adds / changes functionality. Not just to help me understand what you're changing, but to help people in the future. If there are bugs in your patch, I don't want people to have to wade through a 40k diff to find them.

• 

  Jon Leighton October 12th, 2010 @ 07:48 PM

  Ok, no worries. I will split it up, hopefully within the next 24 hours.

• 

  Jon Leighton October 13th, 2010 @ 12:32 PM

  Okay, here is the split up patch. There are actually four commits now:

  1. Add 3 new tests for existing functionality, and fix up other tests affected by changing the fixtures
     
  2. Do the "core changes" talked about here
     
  3. Renaming/formating changes
     
  4. I took the liberty of deleting two methods from JoinAssociation which are not used anywhere.

  I have verified that the entire test suite still succeeds after these changes.

  Hope this helps.

  Cheers

  • join_dependency_refactoring.diff 46.2 KB
• 

  Aaron Patterson October 13th, 2010 @ 04:33 PM

  • State changed from “needs-more-info” to “committed”

  @Jon awesome. Much easier to review. I've applied all your patches to master and pushed. Thank you very much!