Init attributes in InheritedResources controller w/ specs #640

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
3 participants
Collaborator

mikepack commented Jun 5, 2012

Includes the same changes as #546 with additions:

[mikepack: Add specs for resource attributes.]

[mikepack: Remove inconsistent line breaks.]

mccraigmccraig and others added some commits Jan 11, 2012

Add specs for resource attributes.
Remove inconsistent line breaks.
Collaborator

andhapp commented Jun 6, 2012

@mikepack: There seems to be one issue with this pull request.

resource = @controller.send :build_resource
initial_attributes.each do |attr_name, value|
  resource.send("#{attr_name}=", value)
end
resource

This bit of code is exact duplicate from controller_resource superclass. In real life, the superclass code should be called, but in stripped down spec version we stub it. It needs to be refactored to eliminate duplication. Please give your opinion.

Collaborator

mikepack commented Jun 6, 2012

@andhapp I completely agree and was aware of this duplication. The duplication was actually introduced in #546 and I simply added specs. The reason I didn't go about refactoring this is that the code has already been approved by @ryanb and simply needed tests. Moreover, the rate at which @ryanb has been merging pull requests is quite slow so I didn't want to require him to review a refactor just to fix this bug.

If this pull gets merged in, I'll promptly refactor the duplicate code.

Collaborator

andhapp commented Jun 6, 2012

@mikepack: I'd suggest that you should create another pull request without any duplication incorporating the code added in #546. If the pull-request doesn't introduce any major changes and is relatively small in size, then it'll be easily accepted.

Collaborator

mikepack commented Jun 7, 2012

@andhapp Will do. I was looking for the path of least resistance but I agree this should be done right.

Collaborator

andhapp commented Jun 7, 2012

@mikepack: 👍

Collaborator

mikepack commented Jun 8, 2012

@andhapp Refactor complete. Comments welcome. /cc @ryanb

Collaborator

andhapp commented Jun 9, 2012

@mikepack: Doesn't apply cleanly now. Just a suggestion. Close this pull request and then create a fresh one with your specs, refactored code from #546 and I'm sure it'll be okay. Sorry.

Collaborator

mikepack commented Jun 9, 2012

@andhapp Do you mean it can't be merged automatically? It can always be manually merged.

Collaborator

andhapp commented Jun 10, 2012

@mikepack: Surely, it can be merged manually. I will try in in my fork and report any issues here.

Collaborator

andhapp commented Jun 10, 2012

@mikepack : I could merge this manually.

However, I have one question. The following code in inherited_resource:

resource = @controller.send :build_resource
assign_attributes(resource)

defined here will call the superclass build_resource method defined in controller_resource.rb which itself calls assign_attributes(resource) and then we call the same method again in inherited_resource.rb. To conclude, we call the same method twice which doesn't seem right to me.

Please give your opinion.

Collaborator

mikepack commented Jun 11, 2012

@andhapp Thanks for taking a look and testing the merge.

The build_resource method that gets called with @controller.send :build_resource is not the method contained within controller_resource.rb. The build_resource that is called is within the Inherited Resources gem. The naming of these is potentially a coincidence.

Collaborator

andhapp commented Jun 11, 2012

Okay. Thanks for your explanation. I had no idea. Cheers.

On 11 Jun 2012, at 02:15, Mike Packreply@reply.github.com wrote:

@andhapp Thanks for taking a look and testing the merge.

The build_resource method that gets called with @controller.send :build_resource is not the method contained within controller_resource.rb. The build_resource that is called is within the Inherited Resources gem. The naming of these is potentially a coincidence.


Reply to this email directly or view it on GitHub:
#640 (comment)

Collaborator

andhapp commented Jun 18, 2012

@mikepack I just created another pull request (#653) with your changes that can be merged automatically. I didn't want to ask you because I requested you to refactor the code which caused it to diverge and felt I should jump in and fix it. Thanks a lot. Will close this one and ping ryanb to merge another one in.

@andhapp andhapp closed this Jun 18, 2012

Collaborator

mikepack commented Jun 19, 2012

@andhapp No problem. Looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment