Skip to content

Loading…

Extracting a surround with contains does not work #110

Open
pusewicz opened this Issue · 13 comments

6 participants

@pusewicz
  1) Deface::DSL::Loader.extract_dsl_commands_from_erb should work with surround contains
     Failure/Error: dsl_commands.should == "\nsurround 'erb[loud]:contains(\"render :partial => \'spree/admin/shared/configuration_menu\'\")'"
       expected: "\nsurround 'erb[loud]:contains(\"render :partial => 'spree/admin/shared/configuration_menu'\")'"
            got: "\nsurround 'erb[loud]:contains(\"render :partial => \\'\nspree/admin/shared/configuration_menu\\ '\")'" (using ==)
       Diff:
       @@ -1,3 +1,4 @@

       -surround 'erb[loud]:contains("render :partial => 'spree/admin/shared/configuration_menu'")'
       +surround 'erb[loud]:contains("render :partial => \'
       +spree/admin/shared/configuration_menu\ '")'

     # ./spec/deface/dsl/loader_spec.rb:216:in `block (3 levels) in <top (required)>'

Added a test here: pusewicz@45d896b

The regexp for extracting parts does not work well with multiple ' and " in lib/deface/dsl/loader.rb:73

comment.gsub('<!--', '').gsub('-->', '').strip.scan(/[^\s"']+|"[^"]*"|'[^']*'/).each do |part|
@pusewicz pusewicz referenced this issue in spree-contrib/spree_drop_ship
Closed

rails g spree_drop_ship:install blows up on Spree 2.1, Ruby 2 #28

@pusewicz

@radar @BDQ I think this should be resolved before 1.0.0.

@mdundas

+1

I ran into this exact problem with Spree 2.0.3 and spree_drop_ship in debug mode.
I upgraded to Spree 2.1.2 and see the same problem there.

@mdundas

@radar @BDQ Do you guys know of any workaround to this issue?

@radar
Spree Commerce member

@mdundas If we did, we would probably have posted about it here.

Is there a way that I can reproduce this issue on my machine?

@mdundas

If you have a version of Spree that includes Deface 1.0.0 ( like 2.1.x ), then add this to your Gemfile:

gem 'spree_drop_ship', github: 'jdutil/spree_drop_ship'

and then run this:

bundle install
rails g spree_drop_ship:install

Looks like a regex parsing problem in Deface.

@JDutil
Spree Commerce member

I would say this is certainly a regression in Deface, but there does appear to be upgrade notes:
https://github.com/spree/deface#upgrading-from-09-to-10

I haven't tried updating spree_drop_ship for 2.1.x yet, but it is currently using the old 0.9. syntax of code[erb-loud] perhaps if you guys try to update the deface overrides to ensure they are using up to date syntax it might fix the installation process.

It looks like the output in the description @pusewicz has already made the change though so I'm not sure it will actually solve anything.

@BDQ
Spree Commerce member
@JDutil
Spree Commerce member

Nice idea @BDQ thanks.

@pusewicz @mdundas I'll give it a shot to see if I can fix the installation issue for you guys, but please note there are still all the other Rails 4 & Spree 2.1.x changes that will need to be reviewed & fixed as well which I don't have time to work on at the moment. Pull Requests are welcome though!

@mdundas

Thanks so much lovely people, i'll try out this workaround today

@JDutil
Spree Commerce member

Okay guys so I've just pushed an update to spree_drop_ship which just bumps the spree version to 2.1.x and fixes the override by simplifying the contents as @BDQ suggested.

This gets you past the Deface error!

However your now met with the Rails 4 upgrade issues, but I think some of you have already started working on that so please send me some PRs if you get things running for Rails 4.

@mdundas

Thanks @jdutil @BDQ, all good on that workaround ( in Spree 2.1.2, Deface 1.0.0 ) I see a few other spree_drop_issues i'll fork and investigate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.