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

Pass raw params to less parser #116

Closed
wants to merge 7 commits into from
Closed

Conversation

ilyapoz
Copy link
Contributor

@ilyapoz ilyapoz commented Mar 29, 2016

close #94

@ilyapoz
Copy link
Contributor Author

ilyapoz commented Mar 29, 2016

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

@ilyapoz
Copy link
Contributor Author

ilyapoz commented Mar 29, 2016

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

@ilyapoz
Copy link
Contributor Author

ilyapoz commented Apr 2, 2016

@simi ? can we merge this?

@simi
Copy link
Collaborator

simi commented Apr 4, 2016

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

@ilyapoz
Copy link
Contributor Author

ilyapoz commented Apr 4, 2016

@simi did that

@simi
Copy link
Collaborator

simi commented Apr 4, 2016

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
Copy link
Collaborator

simi commented Apr 6, 2016

@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.

@simi simi self-assigned this Apr 6, 2016
@ilyapoz
Copy link
Contributor Author

ilyapoz commented Apr 7, 2016

nah, too late, I got it :)

@ilyapoz
Copy link
Contributor Author

ilyapoz commented Apr 7, 2016

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
Copy link
Contributor Author

ilyapoz commented Apr 7, 2016

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
Copy link
Contributor Author

ilyapoz commented Apr 7, 2016

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?

@@ -45,6 +45,18 @@ class BasicsSpec < Less::Rails::Spec

end

describe 'relative path setting must be effective' do
it 'must use relavite paths by default' do
Rails.application.config.less.raw.relativeUrls = true;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to clean this parameter after test.

@simi
Copy link
Collaborator

simi commented Apr 8, 2016

@ilyapoz it was set here cowboyd/less.rb@4e72e0c#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
Copy link
Collaborator

simi commented Apr 8, 2016

I digged deeper and relativeUrls was introduced in less/less.js@eb5c9fb#diff-41dc76a214b45826bb39925edcccf0f3R19 with false as default and it wasn't changed until today.

@ilyapoz
Copy link
Contributor Author

ilyapoz commented Apr 9, 2016

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
Copy link
Contributor Author

ilyapoz commented Apr 9, 2016

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
Copy link
Collaborator

simi commented Apr 9, 2016

@ilyapoz sonuds reasonable.

@simi
Copy link
Collaborator

simi commented Apr 9, 2016

Merged!

@simi simi closed this Apr 9, 2016
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

Successfully merging this pull request may close these issues.

Less-rails using lessc --relative-urls by default
2 participants