metaskills / less-rails

:-1: :train: Less.js For Rails
http://github.com/metaskills/less-rails
MIT License
340 stars 133 forks source link

Pass raw params to less parser #116

Closed ilyapoz closed 8 years ago

ilyapoz commented 8 years ago

close #94

ilyapoz commented 8 years ago

I'm just starting out with ruby so if someone would help me with writing tests for this, I would appreciate it.

ilyapoz commented 8 years ago

This is not a failure of this diff, so I suppose this should not block it from merging, #117 created.

ilyapoz commented 8 years ago

@simi ? can we merge this?

simi commented 8 years ago

@ilyapoz can you provide some usecase and mention this change in readme and changelog?

ilyapoz commented 8 years ago

@simi did that

simi commented 8 years ago

Great! I have one idea. What about to write README section as universal way how to pass custom args to lessc and mention this one (relativeUrls) as example?

simi commented 8 years ago

@ilyapoz awesome! It will be really nice to add test. I can handle the ruby part. Can you just post me bare lessc command with for example --relativeUrls and some basic file and how it looks with and without that parameter? I'll handle the rest.

ilyapoz commented 8 years ago

nah, too late, I got it :)

ilyapoz commented 8 years ago

Cool that we added the tests, now it seems that default behavior is dependent on ruby versions (probably less.rb gem version, but I'm not sure). If you have any ideas, I would appreciate it. Meanwhile I must probably dig into less.rb history and take a closer look.

ilyapoz commented 8 years ago

Actually strictly speaking this tests less.rb behavior, not parameters passing, but for ppl using this gem as a blackbox this test should be useful. But I think that now we have a bug, because less output depends on ruby version.

ilyapoz commented 8 years ago

I think this is suspicious, I must be not understanding something about the language or the test setup process as I managed to create an unstable test case. Can I reliably test the default behavior here?

simi commented 8 years ago

@ilyapoz it was set here https://github.com/cowboyd/less.rb/commit/4e72e0c87105f8bc70bdd968c7ac445fc315b5d4#diff-0937a07e2f5bb236b1e021c5e94f047aR5 in less.rb 2.3.0.

Maybe it will make sense to test true and false relativeUrls explicitly instead of relying on less.rb defaults. Except this I think it is ready for merge. Can you confirm?

I think we should bump to 2.8 since it is adding new feature. But I can do it when I'll releasing.

simi commented 8 years ago

I digged deeper and relativeUrls was introduced in https://github.com/less/less.js/commit/eb5c9fbf5daf5e01fbf23d4928dca9abe103f6ed#diff-41dc76a214b45826bb39925edcccf0f3R19 with false as default and it wasn't changed until today.

ilyapoz commented 8 years ago

In less.js it defaults to false, in less.rb to true (just to summarise). Testing explicitly is double edged, I would think that people using less-rails and should the authors of less.rb for whatever reason change the default it will be at least good for you to know and bump the major version. If this is a stupid argument, just let me know, I'm not that experienced in releasing libraries via semver.

ilyapoz commented 8 years ago

So basically I think that it's a good idea to fix implicit guarantees (default behavior) in tests, whether it's a good or bad behavior. Because some users might rely on that, so why not state it in the tests.

simi commented 8 years ago

@ilyapoz sonuds reasonable.

simi commented 8 years ago

Merged!