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

Native Sass function to return list separator #689

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@Snugug
Contributor

Snugug commented Mar 20, 2013

This should return the separator of a list or false if the input isn't a list. Solves #688

@Snugug

This comment has been minimized.

Contributor

Snugug commented Mar 20, 2013

Not entirely sure how I would go about writing this test (functions_test.rb, right?). I'll take a look in the morning when I'm less out of it.

@Snugug

This comment has been minimized.

Contributor

Snugug commented Mar 20, 2013

Updated with tests, naming, and incorporated your comments

@nex3

This comment has been minimized.

Contributor

nex3 commented Mar 22, 2013

I don't see any tests... did you remember to add them to the commit?

@Snugug

This comment has been minimized.

Contributor

Snugug commented Apr 2, 2013

Anything else I've got to do for this? I've run into another instance where I need this working.

nex3 added a commit that referenced this pull request Apr 18, 2013

@nex3 nex3 closed this Apr 18, 2013

@robwierzbowski

This comment has been minimized.

Contributor

robwierzbowski commented Apr 20, 2013

Shouldn't this function return Sass::Script::String.new() instead of String.new? On my machine the function silently fails if the result returned isn't a sass string.

@robwierzbowski

This comment has been minimized.

Contributor

robwierzbowski commented Apr 20, 2013

Also, if you're using variable arguments the list class for the variable argument is ArgList, which is always comma delimited. Right now list-separator() is only checking for the List class and then defaulting to space. A quick example:

@mixin arglist-test(args...) {
  @debug list-separator(args);
}

.test {
  @include arglist-test(this, is, comma, separated); // DEBUG: space
}

Changing

if list.class == Sass::Script::List

to

if list.class == Sass::Script::List or list.class == Sass::Script::ArgList

should produce the correct ArgList result.

@nex3

This comment has been minimized.

Contributor

nex3 commented May 10, 2013

@robwierzbowski wrote:
Shouldn't this function return Sass::Script::String.new() instead of String.new? On my machine the function silently fails if the result returned isn't a sass string.

In this context String resolves to Sass::Script::String, since we're in Sass::Script. It could be more explicit, but it works as-is. If it returned a ::String, it would break when Funcall tried to call #options=. I'll add some explicit validation to the test as well, though (b26d269).

Also, if you're using variable arguments the list class for the variable argument is ArgList, which is always comma delimited. Right now list-separator() is only checking for the List class and then defaulting to space.

Good point. Fixed by 7e51238.

@robwierzbowski

This comment has been minimized.

Contributor

robwierzbowski commented May 10, 2013

Thanks. Re: String.new(), it's working fine for me now. Not sure what the problem was before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment