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

Fix association #60

Merged
merged 1 commit into from
Jan 22, 2018
Merged

Conversation

philipqnguyen
Copy link
Contributor

@philipqnguyen philipqnguyen commented Jan 19, 2018

Fix association
To fix association, we require that association accept a blueprint so that we can serialize
associated objects with the correct blueprint.

@AllPurposeName
Copy link
Contributor

I feel like these two changes are quite distinct. Could we (at least going forward) break these things out into multiple PRs?

Copy link
Contributor

@dlcarter dlcarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with removing Optimizer. It was always a 'sugar-on-top' kind of feature, and it's not achieving its intended goals, so agree with removing it until we can get it right.

value = object.public_send(association_name)
return value if value.nil?
view = options[:view] || :default
options[:blueprint].prepare(value, view_name: view, local_options: local_options)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I'm not sure if you intended this or not, but a side effect of this code is that it makes it so that you now always have to have a Blueprint for every association. I think that could be considered a reasonable tradeoff, but I want to make sure we're explicitly making that decision.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, so I do not understand how it would work if the associated object does not have a blueprint.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am for having the requirement that any object being serialized within this system must use a blueprint... I've personally been bitten by having both AMS and jbuilder for the same resource far too many times to try to fashion some coexistence between any of them.

@philipqnguyen
Copy link
Contributor Author

@AllPurposeName Yes, I can separate the removal of Optimizer into another PR.

Require that association accept a blueprint so that we can serialize
associated objects with the correct blueprint.
@philipqnguyen philipqnguyen force-pushed the remove-broken-optimizer-fix-association branch from 8f06bb4 to 4480097 Compare January 19, 2018 19:52
@philipqnguyen philipqnguyen changed the title Remove broken optimizer fix association Fix association Jan 19, 2018
@philipqnguyen
Copy link
Contributor Author

removal of optimizer has been cherry-picked out into #61

association :vehicles, blueprint: vehicle_blueprint
end
end
it('returns json with association') { should eq(result) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so implicit and confusing. Maybe we should switch to MiniTest 😜

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok i admin i over-utilized some of rspec's "features" that does make it quite implicit. I'll make more explicit specs going forward =D

Copy link
Contributor

@AllPurposeName AllPurposeName left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@philipqnguyen philipqnguyen merged commit b0085d5 into master Jan 22, 2018
@philipqnguyen philipqnguyen deleted the remove-broken-optimizer-fix-association branch January 25, 2018 18:06
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

Successfully merging this pull request may close these issues.

3 participants