at-import / Sassy-Maps

Map helper functions for Sass 3.3 and up
MIT License
66 stars 22 forks source link

Drop Compass Gem Dep #5

Closed metaskills closed 10 years ago

metaskills commented 10 years ago

I am using Bourbon with Susy & Breakpoint. The breakpoint gem has a dependency on the sassy-maps gem and it in turn has a dependency on compass. From what I can tell, compass is not needed for this project at all. I would like to not see Compass in my Gemfile.lock for mostly persnickety reasons. However, I have seen compass hold up my dep graph before too. That said, can this gem drop its deps on compass?

Snugug commented 10 years ago

While I disagree with doing this for persnickety reasons, I am inclined to work on dropping the Compass dependency as it indeed shouldn't require compass

On May 18, 2014, at 1:25 PM, Ken Collins notifications@github.com wrote:

I am using Bourbon with Susy & Breakpoint. The breakpoint gem has a dependency on the sassy-maps gem and it in turn has a dependency on compass. From what I can tell, compass is not needed for this project at all. I would like to not see Compass in Gemfile for mostly persnickety reasons, however, I have seen compass hold up my dep graph before too. That said, can this gem drop its deps on compass?

— Reply to this email directly or view it on GitHub.

metaskills commented 10 years ago

Thanks, would you like me to offer up a PR?

Snugug commented 10 years ago

Yah, happy to have one, but it will need to include Compass support if Compass is available, similar to what Breakpoint does

metaskills commented 10 years ago

@Snugug I started looking at this today and the worked seemed extensive. Are you OK with me doing a PR if I greatly simply this lib and publishing a gem that works with for both Compass and raw Sass using SASS_PATH? This would include changes to the README, etc and likely all the tests. I'm more than comfortable doing so but wanted to check with you first.

Snugug commented 10 years ago

I'd prefer a list of suggested changes before you issue a PR in that case

metaskills commented 10 years ago

That's fair, this is just a cursory look thru. I think it will greatly help the approachability of the project and simplify things too. Thanks for considering!

metaskills commented 10 years ago

I want to point out that adding SASS_PATH during this work likely means that you can safely starting doing an @import "sassy-maps" in the breakpoint source. Knowing that either Ruby or Sass have your back for the load paths.

metaskills commented 10 years ago

Maybe even move this out of the load path too. https://github.com/Team-Sass/Sassy-Maps/blob/0.x.x/sass/example.scss

metaskills commented 10 years ago

@Snugug Do you have a better solution?

metaskills commented 10 years ago

@Snugug I am going to move forward with this and at the minimal use my fork. I will make a PR and you can reject or have me tailor it to your liking, whatever works.

For more context, we are using this awesome library along with Susy, Bourbon, and Breakpoint in an internal Sass lib that is also bundle'able via Ruby Bundler. To keep the project playing nice with either Bower and/or Rubygems, I have avoided vendor'ing libraries like this and setting good semver deps.

The issue as it stands now is that our gem will be bundled in quite a few other host applications. Some are moving away from older versions of compass and Sass, others are on 3.3 and are without compass. I am now seeing issues where I can not bundle to either application because Bundler can not resolve my dep graph. On one side, it is due to compass. On other host apps, it is due to Sass being on RC still. My work will address all of these issues and I do hope that you let me contribute to your awesome work when I am done.

Cheers! Hopefully I'll get back to you soon.

Snugug commented 10 years ago

Sorry I haven't been able to respond. Your suggestions look fine to me. The only thing I'd recommend is instead of rewriting the test suite from scratch, use Navigator.

metaskills commented 10 years ago

@Snugug I just looked at Navigator and it still looks like Compass. FYI, the testing is not hard, many of the things you want are just built right into Ruby/MiniTest now-a-days. Even the diff stuff too. The entire file looks like this, really really clean IMHO.

require 'bundler' ; Bundler.require :development, :test
require 'sassy-maps'
require 'minitest/autorun'

module SassyMaps
  class TestHelper < Minitest::Test

    def test_map_get
      assert_rendered_file '01-map-get'
    end

    def test_map_set
      assert_rendered_file '02-map-set'
    end

    def test_memo
      assert_rendered_file '03-memo'
    end

    private

    def assert_rendered_file(file_name)
      rendered_sass = render_sass_file(file_name)
      control_sass = File.read(tests_control_file(file_name))
      assert_equal rendered_sass, control_sass
    end

    def render_sass_file(file_name)
      options = {syntax: :scss, cache: false, style: :expanded}
      template = File.read(tests_sass_file(file_name))
      Sass::Engine.new(template, options).render
    end

    def tests_sass_file(file_name)
      File.join tests_root, 'tests', "#{file_name}.scss"
    end

    def tests_control_file(file_name)
      File.join tests_root, 'controls', "#{file_name}.css"
    end

    def tests_root
      @tests_root ||= File.expand_path File.join(File.dirname(__FILE__))
    end

  end
end

If a tests fails, Minitest already handles the diff, output, etc. For example:

  1) Failure:
SassyMaps::TestHelper#test_memo [/Users/kencollins/Sassy-Maps/tests/sassy_maps_test.rb:17]:
--- expected
+++ actual
@@ -13,7 +13,7 @@
   _test: \"@include memo-set(test, foo, qux)\";
   _get: \"memo-get(test, foo)\";
   _result: qux;
-  _memo-table: (\"test\": ((\"foo\": qux)));
+  _memo-table: (\"test\": ((\"foo\": demohack)));
 }

 .memo--function--new {
metaskills commented 10 years ago

OK, closing this one out, #6 PR has been made.

metaskills commented 10 years ago

Closing this since all the work is done in #6.