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
Proposal for string.split #3284
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good overall; most of my comments are about changes in what arguments are allowed and some formatting tweaks.
Thanks!
proposal/string-split.md
Outdated
returned in the list: | ||
|
||
- ```scss | ||
string.split($fonts, ', ', 2); // "Helvetica Neue" "Helvetica" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the limit truncate the end of the string, or should it take all the remainder in the last part ? Javascript is one of the few languages implementing the truncation (which is easy to achieve otherwise by doing an unlimited split and slicing the array)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stof Are you saying, should this work by 1) making all the splits and then slicing the array at the limit or 2) only splitting up to the limit and then stopping?
Do you think there would be downsides to taking one approach or the other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, to me, it would be more useful if limit
would work by splitting the string on the limit -1
first separators, with the last item containing all the remaining work.
Your current proposal does the opposite (truncating the remaining parts), and this could be achieved through an unlimited split and then slicing the list (but I just figured out that list.slice
does not exist in Sass).
I remember reading an article a few weeks ago comparing the behavior of the string splitting API of lots of different languages in that regard (but unfortunately, I did not manage to find it back for now to share it here. But AFAIR, most languages are providing the non-truncating version of the API (Javascript being one of the exceptions).
Also, a benefit of the non-truncation is that we don't loose content when splitting the string, so joining the list items with the separator (not exposed as a function in sass:list
either btw) always produces the original string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd lean towards matching the JS behavior here. @nex3: Do you have any thoughts on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @stof here. I think JS's behavior is a misfeature, and we should match other languages in preserving the entire remainder of the string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, let's change this to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jathak I've updated this section (and the examples) to show limit
working as noted in the conversation above. The function should preserve the remainder of the string now.
@mirisuzanne made a function based on these steps that you can see here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay on re-reviewing this!
proposal/string-split.md
Outdated
returned in the list: | ||
|
||
- ```scss | ||
string.split($fonts, ', ', 2); // "Helvetica Neue" "Helvetica" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd lean towards matching the JS behavior here. @nex3: Do you have any thoughts on this?
All good @jathak. I'm back in the office in another week-and-a-half or so and I'll adress your latest review then! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just realized that I left comments, but never submitted a formal review. The changes necessary now are to change the limit parameter so it adds the remaining text to the last item, and to explicitly specify that it returns an unbracketed, comma-separated list.
Thanks @jathak . I'm currently working on edits based on re-working how |
@jathak I just wanted to give you a heads up that I will be away starting next week and back around early August. I can respond to your upcoming review then, or if something is urgent, feel free to reach out to the OddBird team in the meantime. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the long delay on reviewing this!
The logic here all looks good, my only remaining comments are formatting/eliminating redundancies.
proposal/string-split.md
Outdated
|
||
* If `$string` is not a string, throw an error. | ||
|
||
* If `$separator` is `null` or is not a string, throw an error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* If `$separator` is `null` or is not a string, throw an error. | |
* If `$separator` is not a string, throw an error. |
null
is already not a string, so we don't need to specifically call it out here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed the null
reference here.
proposal/string-split.md
Outdated
|
||
* Let `split-counter` equal 0. | ||
|
||
* While `split-counter` is `<=` `limit` and `length` is `>` 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* While `split-counter` is `<=` `limit` and `length` is `>` 0: | |
* While `split-counter <= limit` and `length > 0`: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
proposal/string-split.md
Outdated
|
||
* While `split-counter` is `<=` `limit` and `length` is `>` 0: | ||
|
||
* If `split-counter` `==` `limit`: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* If `split-counter` `==` `limit`: | |
* If `split-counter == limit`: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
proposal/string-split.md
Outdated
* Let `limit` be the value of `$limit`. | ||
|
||
* Otherwise, if `$limit` is `null`, set `limit` to the value of `length`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably simpler to just say:
* If `$limit` is `null`, set `$limit` to the value of `length`.
and then refer to $limit
below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
proposal/string-split.md
Outdated
|
||
* Increase `split-counter` by 1. | ||
|
||
* Set `length` to `string.length($string)`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we have to update it after each change to $string
anyway, and length
is only referenced once each loop, I think it makes more sense to eliminate the length
variable entirely and just call string.length($string)
when we need it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I removed the length
variable and inserted calls to string.length($string)
where needed.
Hi @jathak! These changes should cover the last bit of cleanup you suggested. Ready for another look whenever you get a chance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for all your work on this! Sorry it's taken so long to get approved
Proposal for adding
split()
to theStrings
module based on Issue #1950