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

Feature request: support for multiple original hashes #212

Open
brchristian opened this issue Sep 29, 2020 · 2 comments
Open

Feature request: support for multiple original hashes #212

brchristian opened this issue Sep 29, 2020 · 2 comments

Comments

@brchristian
Copy link

brchristian commented Sep 29, 2020

There is a tension that I see in the design of Spree/Solidus extensions, which is that extension maintainers increasingly want a single master branch compatible with many (ideally, all) currently supported versions of Spree/Solidus, rather than having a separate v2.10, v2.11, v3.0, etc. branch for each release.

I generally think that this makes sense and is a good practice.

However there is a problem with this in that the Deface override hashes are likely to be different across those versions. Some maintainers refuse to use original hashes for this reason. Users' logs will fill up with Deface: [WARNING] No :original defined messages.

Other maintainers decide which branch of Spree/Solidus to track. For all other versions of Spree/Solidus, the overrides still work, but those users' logs fill up with Deface: [ERROR] The original source has changed.

What if there was a better way?

Well, it occurs to me that the simplest solution here is for original to take an array of hashes (or alternately, for it to be possible to define original multiple times), and if any of those hashes match, no error or warning is thrown.

This way maintainers could simply include one hash for each branch; one hash for the 2.10 release, one for the 2.11 release, and one for the 3.0 release, etc.

What do you think?

If there is interest in merging a feature like this, I would be happy to prepare a PR.

@brchristian
Copy link
Author

Additionally, I sometimes see in my logs a case where an override matches more than once, and each match has its own hash. So my logs look like this:

Deface: 'add_widget_to_subscriber_lists' matched 2 times with '[data-hook="subscriber_list_row"]'
Deface: [WARNING] No :original defined for 'add_widget_to_subscriber_lists', you should change its definition to include:
 :original => 'c5bd5b3ec852c7479f311caf876c6d40e85b26eb' 
Deface: [WARNING] No :original defined for 'add_widget_to_subscriber_lists', you should change its definition to include:
 :original => '6505f8b2f8aef5f74b5357fb0e5afaf11d6ba431' 

Here, no matter which of these hashes I put in my Deface override, the other one will throw an error.

This is very frustrating. Again, I believe that extending original to accept multiple hashes will solve this problem in a very straightforward/elegant way.

@aldesantis
Copy link
Collaborator

@brchristian I think accepting an array of original hashes is a very good idea! 👍 for me.

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

No branches or pull requests

2 participants