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

Refactor calculators to be more opinionated what to compute #4041

Closed
wants to merge 15 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@peterberkenbosch
Member

peterberkenbosch commented Dec 2, 2013

Might need some more edge test cases.

@huoxito

This comment has been minimized.

Show comment
Hide comment
@huoxito

huoxito Nov 27, 2013

I don't get why we would still need the raise here? The idea looks good btw

I don't get why we would still need the raise here? The idea looks good btw

This comment has been minimized.

Show comment
Hide comment
@peterberkenbosch

peterberkenbosch Nov 27, 2013

Owner
Owner

peterberkenbosch replied Nov 27, 2013

Show outdated Hide outdated core/app/models/spree/calculator.rb
# Spree::LineItem -> :compute_line_item
computable_name = computable.class.name.demodulize.underscore
method = "compute_#{computable_name}".to_sym
the_caller = caller[0].split(':')[0]

This comment has been minimized.

@radar

radar Dec 2, 2013

Member

Interesting use of caller here.

@radar

radar Dec 2, 2013

Member

Interesting use of caller here.

This comment has been minimized.

@peterberkenbosch

peterberkenbosch Dec 5, 2013

Member

Yes, also totally wrong! Was thinking the caller was the subclass, but was basically the spec or controller. Have other solution for this in the works though.

@peterberkenbosch

peterberkenbosch Dec 5, 2013

Member

Yes, also totally wrong! Was thinking the caller was the subclass, but was basically the spec or controller. Have other solution for this in the works though.

Show outdated Hide outdated core/app/models/spree/shipping_calculator.rb
compute_package package
def compute_shipment(shipment)
compute(shipment.to_package)

This comment has been minimized.

@radar

radar Dec 3, 2013

Member

Interesting that this calculator went from taking either a package or a shipment to now just taking a shipment that's turned into a package. Why is that? What was passing in a package before?

@radar

radar Dec 3, 2013

Member

Interesting that this calculator went from taking either a package or a shipment to now just taking a shipment that's turned into a package. Why is that? What was passing in a package before?

This comment has been minimized.

@peterberkenbosch

peterberkenbosch Dec 3, 2013

Member

It supports both calls, for a shipment or a package. The previous code just got the package from the shipment and computed that. (see https://github.com/spree/spree/blob/master/core/app/models/spree/shipping_calculator.rb)

@peterberkenbosch

peterberkenbosch Dec 3, 2013

Member

It supports both calls, for a shipment or a package. The previous code just got the package from the shipment and computed that. (see https://github.com/spree/spree/blob/master/core/app/models/spree/shipping_calculator.rb)

This comment has been minimized.

@mrpollo

mrpollo Dec 5, 2013

Contributor

@radar I recall this allowing for an order to be passed as well at some point.

@peterberkenbosch I believe the main use for calculators on shipments are for compute_package specially on spree_active_shipping, I advice care if you use the different implementations for the same thing which is calling compute because of the different overrides we might find already implemented, I know that this happened before when we went from compute to compute_package on spree_active_shipping, what's stopping us from doing compute_package(shipment.to_package) in here? that might help with this, unless I'm totally missing the point which might be the case :trollface:

@mrpollo

mrpollo Dec 5, 2013

Contributor

@radar I recall this allowing for an order to be passed as well at some point.

@peterberkenbosch I believe the main use for calculators on shipments are for compute_package specially on spree_active_shipping, I advice care if you use the different implementations for the same thing which is calling compute because of the different overrides we might find already implemented, I know that this happened before when we went from compute to compute_package on spree_active_shipping, what's stopping us from doing compute_package(shipment.to_package) in here? that might help with this, unless I'm totally missing the point which might be the case :trollface:

This comment has been minimized.

@peterberkenbosch

peterberkenbosch Dec 6, 2013

Member

good point @mrpollo the current implementation is aiming on keeping the original specs pass. But I see what you meen, the compute_shipment can only work if the shipment actually responds to to_package which is not very clear.

@peterberkenbosch

peterberkenbosch Dec 6, 2013

Member

good point @mrpollo the current implementation is aiming on keeping the original specs pass. But I see what you meen, the compute_shipment can only work if the shipment actually responds to to_package which is not very clear.

@peterberkenbosch

This comment has been minimized.

Show comment
Hide comment
@peterberkenbosch

peterberkenbosch Dec 4, 2013

Member

Work on this targeted for #3567

Member

peterberkenbosch commented Dec 4, 2013

Work on this targeted for #3567

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Dec 8, 2013

Member

@peterberkenbosch Is this good to merge yet?

Member

radar commented Dec 8, 2013

@peterberkenbosch Is this good to merge yet?

@peterberkenbosch

This comment has been minimized.

Show comment
Hide comment
@peterberkenbosch
Member

peterberkenbosch commented Dec 9, 2013

:shipit: 👍

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Dec 10, 2013

Member

:shipit: 💥

Member

radar commented Dec 10, 2013

:shipit: 💥

@peterberkenbosch

This comment has been minimized.

Show comment
Hide comment
@peterberkenbosch

peterberkenbosch Dec 14, 2013

Member

this is merged in master already (thanks @radar) so closing this PR.

Member

peterberkenbosch commented Dec 14, 2013

this is merged in master already (thanks @radar) so closing this PR.

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