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

Native Sass function to return list separator #689

Closed
wants to merge 4 commits into from
Closed

Native Sass function to return list separator #689

wants to merge 4 commits into from

Conversation

Snugug
Copy link

@Snugug 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
Copy link
Author

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

Snugug commented Mar 20, 2013

Updated with tests, naming, and incorporated your comments

@nex3
Copy link
Contributor

nex3 commented Mar 22, 2013

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

@Snugug
Copy link
Author

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 pushed a commit that referenced this pull request Apr 18, 2013
@nex3 nex3 closed this Apr 18, 2013
@robwierzbowski
Copy link

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

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

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants