Move decorators to Module.prepend for Zeitwerk compatibility#2
Open
spaghetticode wants to merge 3 commits intosolidus-3from
Open
Move decorators to Module.prepend for Zeitwerk compatibility#2spaghetticode wants to merge 3 commits intosolidus-3from
spaghetticode wants to merge 3 commits intosolidus-3from
Conversation
Besides replacing class_eval with prepend, this change is adding the SpreePriceBooks namespace to these decorators to ensure their class name remains unique.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors decorator files to use Module.prepend instead of class_eval for Zeitwerk compatibility. The changes organize decorators within the Spree::SpreePriceBooks namespace to ensure unique class names and prevent potential conflicts in autoloading systems.
Key changes include:
- Converting all
class_evaldecorators to module-based decorators usingprepend - Moving decorators to the
Spree::SpreePriceBooksnamespace - Relocating files to maintain proper namespacing structure
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| app/models/spree/store_decorator.rb | Removed original store decorator using class_eval |
| app/models/spree/spree_price_books/variant_decorator.rb | Converted to module-based decorator with prepend pattern |
| app/models/spree/spree_price_books/user_decorator.rb | Converted to module-based decorator with prepend pattern |
| app/models/spree/spree_price_books/store_decorator.rb | Added new namespaced store decorator using prepend |
| app/models/spree/spree_price_books/role_decorator.rb | Added new namespaced role decorator using prepend |
| app/models/spree/spree_price_books/product_decorator.rb | Converted to module-based decorator with prepend pattern |
| app/models/spree/spree_price_books/price_decorator.rb | Added new namespaced price decorator using prepend |
| app/models/spree/role_decorator.rb | Removed original role decorator using class_eval |
| app/models/spree/price_decorator.rb | Removed original price decorator using class_eval |
| app/models/spree/order/spree_price_books/currency_updater_decorator.rb | Added new namespaced currency updater decorator |
| app/models/spree/order/currency_updater_decorator.rb | Removed original currency updater decorator using class_eval |
| app/helpers/spree/spree_price_books/base_helper_decorator.rb | Added new namespaced base helper decorator |
| app/helpers/spree/base_helper_decorator.rb | Removed original base helper decorator using class_eval |
| app/controllers/spree/admin/spree_price_books/products_controller_decorator.rb | Added new namespaced products controller decorator |
| app/controllers/spree/admin/products_controller_decorator.rb | Removed original products controller decorator using class_eval |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Comment on lines
+4
to
+6
| -> { where currency: Spree::Config[:currency], price_book_id: Spree::PriceBook.default.id }, | ||
| class_name: 'Spree::Price', | ||
| dependent: :destroy |
There was a problem hiding this comment.
[nitpick] The association definition spans multiple lines but uses inconsistent indentation. Consider aligning the lambda block parameters consistently with the association declaration.
Suggested change
| -> { where currency: Spree::Config[:currency], price_book_id: Spree::PriceBook.default.id }, | |
| class_name: 'Spree::Price', | |
| dependent: :destroy | |
| -> { where currency: Spree::Config[:currency], price_book_id: Spree::PriceBook.default.id }, | |
| class_name: 'Spree::Price', | |
| dependent: :destroy |
| class_name: 'Spree::Price', | ||
| dependent: :destroy | ||
|
|
||
| base.has_many :prices, |
There was a problem hiding this comment.
[nitpick] Similar to the previous association, the multi-line association definition should maintain consistent indentation for better readability.
- Fix initializer name from "spree_active_shipping.register.calculators" to "spree_price_books.register.calculators" (correct gem name) - Add "after: spree.register.calculators" dependency to ensure proper initialization order - Wrap calculator registration in after_initialize block to ensure Rails is fully initialized before registering calculators, which is required for Rails 7 and Zeitwerk autoloader compatibility
Fix calculator registration for Rails 7 and Zeitwerk compatibility
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Besides replacing class_eval with prepend, this change is adding the SpreePriceBooks namespace to these decorators to ensure their class name remains unique.