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

Do not require view overrides #5990

Merged

Conversation

sauloperez
Copy link
Contributor

What? Why?

View overrides were removed long ago, so no need to require an empty list.

The next step will be class decorators.

What should we test?

A green build is enough.

Release notes

Do not require long gone view overrides
Changelog Category: Removed

View overrides were removed long ago, so no need to require an empty
list.

The next step will be class decorators.
@sauloperez sauloperez self-assigned this Sep 4, 2020
Copy link
Contributor

@luisramos0 luisramos0 left a comment

Choose a reason for hiding this comment

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

😍 I spotted that one yesterday working on bye bye spree.

@sauloperez
Copy link
Contributor Author

I'll cede the removal of the decorators to you, as a humble recognition 😂

@sauloperez sauloperez merged commit c7bcd61 into openfoodfoundation:master Sep 4, 2020
@sauloperez sauloperez deleted the dont-require-overrides branch September 4, 2020 15:20
@luisramos0
Copy link
Contributor

ehehe :-D
There are decorators in spree_paypal_express, so I think we will have that in here for while. Even after spree_core is gone.

@sauloperez
Copy link
Contributor Author

sauloperez commented Sep 8, 2020

any reason why we couldn't bring in spree_paypal_express? We're maintaining our own fork anyway.

@luisramos0
Copy link
Contributor

I like it to be a separate gem actually! Paypal is really an addon. Maybe we can do the opposite in this case and move what's in the decorator to the gem?

@sauloperez
Copy link
Contributor Author

IMO having it in the same codebase would make it much easier to deal with such an updated integration. Remember we went through this summer. A separate gem makes sense for a product like Spree but I'm not that sure for something like OFN after Spree is merged in.

@luisramos0
Copy link
Contributor

yeah, it's debatable. Spree is a heavily customized core dependency, doesnt make sense to have it externalized 👍 Paypal is totally different case, I think it's ok to have it as a gem. I am ok with merging it to ofn. But only if integrated in a specific engine, I'd even prefer to have an engine only for it.

@sauloperez
Copy link
Contributor Author

makes sense in an engine, def.

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

Successfully merging this pull request may close these issues.

None yet

2 participants