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

String functions #401

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
@chriseppstein
Member

chriseppstein commented May 31, 2012

This pull adds some string functions that I feel are a fairly complete, but basic set of string manipulation functions.

  1. str-length - the length of a string
  2. str-index - the first index of a substring in another string
  3. str-insert - inserts a string into another string.
  4. str-extract - extract a substring from a string.
  5. to-upper-case - convert a string to uppercase.
  6. to-lower-case - convert a string to lowercase.

All of them are documented and tested. Feedback on the api, etc is welcome. Did I miss any very important string functions?

@micahgodbolt

This comment has been minimized.

micahgodbolt commented May 31, 2012

Can you give a quick example of these in use? Curious how the syntax works.

@StanAngeloff

This comment has been minimized.

StanAngeloff commented May 31, 2012

I sooo needed this a couple of days ago and ended up doing magic with @-functions and @-mixins. Other than the abbreviated prefix (str over string) and personal preferences about naming, I am very pleased with the API. Any methods missing, e.g., str-append or str-prepend can be added in code with no pain.

I'd also like to see list-join which may be out of the scope of this pull? I find myself concatenating lists very often, although a @-function is trivial to implement.

EDIT: come to think of it, list functions would be quite cool. list-index-of, list-push, list-unique come to mind. I may open up a separate issue, don't want to hijack the conversation in here.

EDIT 2: index(..) already exists in Sass.

@chriseppstein

This comment has been minimized.

Member

chriseppstein commented May 31, 2012

@micahgodbolt the tests are a good overview. did you read those yet?

@micahgodbolt

This comment has been minimized.

micahgodbolt commented May 31, 2012

Oh hey...those are links. Guess I should fully explore the page before asking questions. Those should work just fine. Thanks @chriseppstein

@chriseppstein

This comment has been minimized.

Member

chriseppstein commented May 31, 2012

@StanAngeloff I am planning on making a separate pull for lists soon. we already have join for lists.

@StanAngeloff

This comment has been minimized.

StanAngeloff commented May 31, 2012

@chriseppstein: Sounds very promising. RE join: joins two lists together, I should probably have used implode as in join a list and return a string, e.g. implode('.', join((hello), (world), space)) -> 'hello.world'.

I keep a handy _functions.scss around, hopefully lists (and other string extras) can be implemented as pure Sass @-functions. Is that the preferred approach when logic allows it?

@chriseppstein

This comment has been minimized.

Member

chriseppstein commented May 31, 2012

Is that the preferred approach when logic allows it?

@StanAngeloff I'm on the fence about that and am curious how your experience has been. we should chat about that in a separate thread.

@chriseppstein

This comment has been minimized.

Member

chriseppstein commented Aug 8, 2012

We put this off because 3.2 was going to ship soon. @nex3, let's ship or merge this.

@chriseppstein

This comment has been minimized.

Member

chriseppstein commented Aug 13, 2012

@nex3 I will add a CHANGELOG entry for 3.3 for this. Any further comment before it lands on master?

@nex3

This comment has been minimized.

Contributor

nex3 commented Aug 15, 2012

Beginning code review now.

@nex3

This comment has been minimized.

Contributor

nex3 commented Aug 15, 2012

All of these functions should have @param documentation for all the parameters, at least to specify the types and also to specify whatever other constraints/non-obvious semantics exist.

@nex3

This comment has been minimized.

Collaborator

nex3 commented on lib/sass/script/functions.rb in 934f511 Aug 15, 2012

You don't need to fully qualify these names, and it'll make the docs look nicer if you don't.

@nex3

This comment has been minimized.

Collaborator

nex3 commented on lib/sass/script/functions.rb in 347dbca Aug 15, 2012

The summary for a function shouldn't mention the arguments by name. "Inserts one string into another" would be better.

@nex3

This comment has been minimized.

Collaborator

nex3 commented on lib/sass/script/functions.rb in 347dbca Aug 15, 2012

Proper capitalization and punctuation.

@nex3

This comment has been minimized.

Collaborator

nex3 commented on lib/sass/script/functions.rb in 347dbca Aug 15, 2012

"will start", although I'm not sure how useful this sentence is given that you already said that it inserts the string at the given index.

@nex3

This comment has been minimized.

Collaborator

nex3 commented on lib/sass/script/functions.rb in 347dbca Aug 15, 2012

Here it makes sense to refer to a variable by name (e.g. $insert, $index).

@nex3

This comment has been minimized.

Collaborator

nex3 commented on lib/sass/script/functions.rb in 347dbca Aug 15, 2012

This is the sort of thing that should go in parameter documentation.

@nex3

This comment has been minimized.

Collaborator

nex3 commented on lib/sass/script/functions.rb in 347dbca Aug 15, 2012

Here you could make it clear that this doesn't destructively modify $original, but instead returns a new string.

@nex3

This comment has been minimized.

Collaborator

nex3 commented on lib/sass/script/functions.rb in 347dbca Aug 15, 2012

This is too many examples. You don't need to make every test case an example; it'll overwhelm the casual reader. Just one or two is enough.

The same goes for some of the other functions.

This comment has been minimized.

Owner

chriseppstein replied Aug 16, 2012

I think this documentation is useful and shouldn't be removed. I'm not making test cases an example, I'm demonstrating how the edge cases behave to the user. We can trim down the docs if users complain.

This comment has been minimized.

Collaborator

nex3 replied Aug 20, 2012

@example is not the place to explain every edge case of the function. It exists primarily for the reader who doesn't want to expend energy actually reading the documentation and type annotations, and who just wants to copy a snippet of code. Making them wade through many lines of probably-irrelevant snippets to get to one or two relevant ones runs counter to that goal.

@nex3

This comment has been minimized.

Collaborator

nex3 commented on lib/sass/script/functions.rb in 347dbca Aug 15, 2012

Validate that this is an integer (same for similar arguments in other functions).

@nex3

This comment has been minimized.

Collaborator

nex3 commented on 347dbca Aug 15, 2012

What's the use case for this function?

This comment has been minimized.

Owner

chriseppstein replied Aug 16, 2012

My goal of these functions was not to satisfy a particular stylesheet authoring use case. But rather create a minimal but very usable set of string manipulation functions based on my programming experience. I added this function because it's much easier to use than concatenating a bunch of extracted substrings.

This comment has been minimized.

Collaborator

nex3 replied Aug 20, 2012

I rarely use this functionality in Ruby, let alone Sass. It doesn't even exist in Javascript. I don't think it's widely applicable enough to add to core.

This comment has been minimized.

Owner

chriseppstein replied Aug 20, 2012

I use it. Please don't overgeneralize your own experience.

@nex3

This comment has been minimized.

Collaborator

nex3 commented on lib/sass/script/functions.rb in a9c94b0 Aug 15, 2012

Mention that this will return 0 if the substring isn't found.

@nex3

This comment has been minimized.

Collaborator

nex3 commented on lib/sass/script/functions.rb in 640abf6 Aug 15, 2012

Probably didn't intend to delete this ;)

This comment has been minimized.

Owner

chriseppstein replied Aug 16, 2012

good catch.

@nex3

This comment has been minimized.

Collaborator

nex3 commented on lib/sass/script/functions.rb in a07a585 Aug 15, 2012

Use @overload to make it look like this takes parameters named start and end, and to document that end_at defaults to null. Also, have it default to the SassScript null value. Or even better, make it default to -1.

Also, document what it means for start_at and end_at to be negative, or for end_at not to be passed.

@nex3

This comment has been minimized.

Collaborator

nex3 commented on a07a585 Aug 15, 2012

It seems like this should probably be called substring, since that's what most languages (JS in particular) call it.

This comment has been minimized.

Owner

chriseppstein replied Aug 16, 2012

I'm ok with renaming it, my rationale was to have a consistent prefix of str- on the string functions and str-substring was silly. though, the upcase and downcase functions don't comply with this pattern...

This comment has been minimized.

jkmaxwell replied Dec 13, 2012

Agreed this should be substring

This comment has been minimized.

Owner

chriseppstein replied Feb 18, 2013

Javascript has two methods: substring and slice. substring doesn't accept negative values so I decided to call this str-slice which is consistent with both the javascript method and the naming convention.

@nex3

This comment has been minimized.

Collaborator

nex3 commented on test/sass/functions_test.rb in a07a585 Aug 15, 2012

I don't like this behavior. This is the only case where the function slices from the right as opposed to from the left, and it's the only case where $end doesn't default to -1. It's also easy to fake this without building it in, since the current behavior of str-extract($str, -$x) is equivalent to str-extract($str, 1, -$x).

This comment has been minimized.

Owner

chriseppstein replied Aug 16, 2012

Fair enough.

@nex3

This comment has been minimized.

Contributor

nex3 commented Aug 15, 2012

Code review done.

@chriseppstein

This comment has been minimized.

Member

chriseppstein commented Aug 16, 2012

My response to the code review is complete. If I didn't comment on a particular comment, it means I acknowledge it and will address it as suggested.

@nex3

This comment has been minimized.

Contributor

nex3 commented Aug 20, 2012

I'll merge this in once you address the minor comments.

@industral

This comment has been minimized.

industral commented Oct 4, 2012

could you plz merge it, I need that functionality :) @chriseppstein thanks for good work!

@domenic

This comment has been minimized.

domenic commented Dec 7, 2012

Argh I need this too.

@jkmaxwell

This comment has been minimized.

jkmaxwell commented Dec 13, 2012

@chriseppstein any chance to address the comments (especially the deletion of what looks like some critical code in the case switching) so this can get merged?

(also thanks, this is great!)

@ghost

This comment has been minimized.

ghost commented Dec 18, 2012

Whats the latest on this? Soon to be merged? So need this one :)
kinda annoying to know this is all neatly done just hanging in the air atm

@adamcoulombe

This comment has been minimized.

adamcoulombe commented Dec 28, 2012

Need this to create keyframe template mixins. Need this merged!!

@seafoox

This comment has been minimized.

seafoox commented Jan 17, 2013

+1, would be very useful. Thx

@jkmaxwell

This comment has been minimized.

jkmaxwell commented Jan 17, 2013

@chriseppstein please address comments

@brentd

This comment has been minimized.

brentd commented Feb 9, 2013

@chriseppstein Would be great to see this merged. How can I help? Happy to take on the needed code changes.

@chriseppstein

This comment has been minimized.

Member

chriseppstein commented Feb 18, 2013

All feedback has been addressed and the string functions have been merged to master.

@baseten

This comment has been minimized.

baseten commented Sep 24, 2013

In SASS 3.3.0.alpha.149 these string functions are present, but str-extract refuses to work, it doesn't throw an error but simply outputs the function call as a string. I've tried the latest alpha (3.3.0.alpha.255 at time of writing) and this still happens.

@baseten

This comment has been minimized.

baseten commented Sep 24, 2013

Appears to have been renamed str-slice...

@Anahkiasen

This comment has been minimized.

Anahkiasen commented Nov 13, 2013

Is there an str-replace function or something similar ?

@rvinay88

This comment has been minimized.

rvinay88 commented Nov 19, 2013

str-extract is not working for me when I use SassMeister, which has 3.3.0.rc.1
https://gist.github.com/rvinay88/7553853

@nex3

This comment has been minimized.

Contributor

nex3 commented Dec 6, 2013

@Anahkiasen: not currently.

@rvinay88: it was renamed to str-slice.

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