Skip to content
This repository has been archived by the owner on Mar 5, 2021. It is now read-only.

adding 'to-string' function #84

Open
wants to merge 29 commits into
base: master
Choose a base branch
from
Open

Conversation

esr360
Copy link
Contributor

@esr360 esr360 commented Sep 4, 2015

So I just realised that SassyJSON does not come with your 'to-string' function - I just happened to already be using it for something else which is why it worked for me. And of course my previous pull-request relies on this function.

I've tried to be as consistent as possible when creating the new file but please let me know if you need any changes.

@KittyGiraudel
Copy link
Owner

@esr360
Copy link
Contributor Author

esr360 commented Sep 8, 2015

Hmm, that uses the "_proof-quote" function, which is the function in which we are trying to convert the value to a string. As a result, I'm getting "SystemStackError: stack level too deep".

@KittyGiraudel
Copy link
Owner

Ouch... That doesn't sound to good. I will have a closer look.

Le mar. 8 sept. 2015 21:28, Edmund [email protected] a écrit :

Hmm, that uses the "_proof-quote" function, which is the function in which
we are trying to convert the value to a string. As a result, I'm getting
"SystemStackError: stack level too deep".


Reply to this email directly or view it on GitHub
#84 (comment)
.

// @param {String} $glue ('') - value to use as a join string

@function to-string($list, $glue: '', $is-nested: false, $recursive: false) {
$result: null;
Copy link
Owner

Choose a reason for hiding this comment

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

Should be ''.

Copy link
Owner

Choose a reason for hiding this comment

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

Too large indentation, I'd say?

@esr360
Copy link
Contributor Author

esr360 commented Sep 10, 2015

I've made the requested changes, and it's working perfectly on my local project.

Just one thing regarding this updated line:

$result + #{$e} + #{$glue}, $result#{$e}

Perhaps this is expected, but this doesn't work:

$result + #{$e} + #{$glue}, $result + #{$e}

It outputs the '+' symbol.

@KittyGiraudel
Copy link
Owner

If you concatenate with +, don't interpolate.

@esr360
Copy link
Contributor Author

esr360 commented Sep 11, 2015

Ok, I think we got there :)

My butchered attempt at concatenating was causing issues when using '' instead of null for the $result. Fixing the concatenation consequently meant that using $result: ''; now works as expected (and vice versa - the presence of $result: null was messing up the concatenation - both needed to be done simultaneously).

@KittyGiraudel
Copy link
Owner

Weird, a test is failing.

@for $i from 1 through length($list) {
$e: nth($list, $i);
@if type-of($e) == 'list' and $recursive {
$result: $result + #{to-string($e, $glue, true, true)};
Copy link
Owner

Choose a reason for hiding this comment

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

No need for interpolation here. :)

Copy link
Owner

Choose a reason for hiding this comment

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

$is-nested and $recursive should be passed rather than true and true.

@esr360
Copy link
Contributor Author

esr360 commented Sep 11, 2015

Done and done :) Test still seems to be failing though?

Cheers

@KittyGiraudel
Copy link
Owner

Not quite sure why... Might be just the linting.

@KittyGiraudel
Copy link
Owner

Actually I have been double-checking this and I think we don't need this to-string at all. The proof-quote function is only called in safe circumstances where the value can be safely quoted without having to be stringified first.

@esr360
Copy link
Contributor Author

esr360 commented Sep 14, 2015

o_O Well my project still works as expected after removing this, but I was definitely having issues which converting the value to a string fixed. I can't right now replicate it, but I believe it was when trying to pass a value of 'null' to the function, resulting in 'Invalid null operation: """ plus null".'.

@KittyGiraudel
Copy link
Owner

For now I'd say the only thing we need is getting rid of this to-string(..) call.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants