305064 - Add trim, trimLeft, and trimRight features for javascript strings

Status: VERIFIED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Egon Knapen]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6

For example:
.Trim() is a function I use a lot, it would be nice to have a native version for
the String class in JavaScript. Maybe with or without a .LTrim() or .RTrim()
alternativ.


Reproducible: Always




After the news of added functions for the Array class, it would be nice if it
were possible to extend the javascript String class also.

I use a lot of PHP + javascript, and there I noticed that a lot of functions for
string manupulation under PHP are missing in javascript. Trim is just the most
missed function.

Comment 1

[Brendan Eich [:brendan]]

Shaver, should I use wiki.mozilla.org or developer.mozilla.org to catalog,
judge, modify, and select or reject new native methods?

/be

Comment 2

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

wiki to design, devmo to document.

Comment 3

[Brendan Eich [:brendan]]

JS follows Java which followed Smalltalk in preferring fairly verbose, camelCaps method names. So trimLeft and trimRight would be better names. I'm not convinced they're really worth it, given most trim use-cases want to get rid of any leading or trailing whitespace, but we should try to name things conventionally.

/be

Comment 4

[Egon Knapen]

Just .trim() would be enough, any .trim() would be fine... I personally never use the ltrim or rtrim functions either, only trim()

Comment 5

[Damon Sicore (:damons)]

Adding this to the 1.9.1 triage queue by marking this wanted1.9.1?.  If this should be a blocker, please mark accordingly.

Comment 6

[Egon Knapen]

I don't think this should be a blocker, ist just a enhancement request. Because a lot of libraries have such a function, I think it would be nice to have it nativ in javascript someday...

Comment 7

[Mike Kaplinskiy [:mikekap] (at school, no time)]

Attachment: Patch v1

Comment 8

[Robert Sayre]

Comment on attachment 329909 [details] [diff] [review]
Patch v1

Seems like you could share more code by changing the signature of js_TrimString to return JSBool and passing it vp.

Comment 9

[Mike Kaplinskiy [:mikekap] (at school, no time)]

Attachment: Patch v2

Addressing sayrer's comments. I think at some point I considered adding js_TrimString to the header.

Should there be tests for this? And if so, where/how should I add them?

Comment 10

[Igor Bukanov]

Comment on attachment 330131 [details] [diff] [review]
Patch v2

 
>+static JSBool
>+js_TrimString(JSContext *cx, jsval *vp, JSBool trimLeft, JSBool trimRight)
>+{
>+    JSString *str;
>+    const jschar *chrs;

Nit: the canonical name is chars, not chrs.

>+    jsint len, begin, end;

Use size_t, not jsint, for the bounds as the string length is size_t. This will require to change the code to use end = len etc. but that would match the typical meaning of end in the code as the one-past-maximum value.

Comment 11

[Mike Kaplinskiy [:mikekap] (at school, no time)]

Attachment: Patch v3, addressing igor's comments

Comment 12

[Robert Sayre]

(In reply to comment #9)
> 
> Should there be tests for this? And if so, where/how should I add them?

We have a big test suite here:
http://mxr.mozilla.org/mozilla-central/source/js/tests/

Unfortunately, I don't have good instructions available for adding one to it. Just attaching a sample js file that compares some expected vs. actual results would be great.

Comment 13

[Igor Bukanov]

Comment on attachment 330145 [details] [diff] [review]
Patch v3, addressing igor's comments

> 
>+static JSBool
>+js_TrimString(JSContext *cx, jsval *vp, JSBool trimLeft, JSBool trimRight)
>+{
>+    JSString *str;
>+    const jschar *chars;
>+    size_t len, begin, end;

Final nit: jsstr.cpp uses either length or n, not len, for the length variable.

Comment 14

[Robert Sayre]

Mike, please attach a patch covering Igor's final nit, and we'll get this landed. Thanks!

Comment 15

[Mike Kaplinskiy [:mikekap] (at school, no time)]

Attachment: Patch v4

Comment 16

[Biju]

1.
Just curious, whether Trim also removes any leading/trailing white spaces 
or just ascii 32 spaces?


2.
What do trimLeft do on a right-to-left text?
isn't better to name trimStart/trimEnd like in .Net
http://msdn.microsoft.com/en-us/library/system.string.trimstart.aspx
there are a lot of .Net programmers doing web dev now, it make easy for them.


3.
.Net trim/trimStart/trimEnd  also has a feature to take a char array as parameter to remove other char
http://msdn.microsoft.com/en-us/library/system.string.trim.aspx
it will be better if we add that feature too.


4.
I am a old programmer who came thru BASIC, FORTRAN, ASM. 
And also very poor on spellings.
So I love short names. 
For centuries BASIC programs had "ltrim" and "rtrim".
Having short names in JavaSript would make thing easier 
when writing BookMarklets.
So can we have shorter names?


5.
There also other useful methods
http://msdn.microsoft.com/en-us/library/system.string_members.aspx
like StartsWith/EndsWith, IndexOfAny, 
PadLeft/PadRight (or better PadStart/PadEnd/PadCenter)

Comment 17

[Bob Clary [:bc] (inactive)]

(In reply to comment #12)

> 
> Unfortunately, I don't have good instructions available for adding one to it.
> Just attaching a sample js file that compares some expected vs. actual results
> would be great.
> 

I'll add a document to developer.mozilla.org today about how to add JavaScript tests.

Comment 18

[Mike Kaplinskiy [:mikekap] (at school, no time)]

Attachment: Test case for trim

To be run in the js console and should print true 6 times. This doesn't test all the characters that trim should pick up as whitespace (I'll attach one more)

(In reply to comment #16)

> 1.
> Just curious, whether Trim also removes any leading/trailing white spaces 
> or just ascii 32 spaces?

The implementation should produce the same results as string.replace(/^\s+|\s+$/g, ''), as per ES 3.1 draft. http://wiki.ecmascript.org/doku.php?id=es3.1:targeted_additions_to_array_string_object_date (So tabs, newlines, and every 'empty' character should match)
 
> 2.
> What do trimLeft do on a right-to-left text?
> isn't better to name trimStart/trimEnd like in .Net
> http://msdn.microsoft.com/en-us/library/system.string.trimstart.aspx
> there are a lot of .Net programmers doing web dev now, it make easy for them.

I guess it wouldn't hurt, but IE doesn't even implement regular trim, so this is more likely to be used in firefox internals (bug 20348). If Igor doesn't have any objections, I could change the names.

> 3.
> .Net trim/trimStart/trimEnd  also has a feature to take a char array as
> parameter to remove other char
> http://msdn.microsoft.com/en-us/library/system.string.trim.aspx
> it will be better if we add that feature too.

Probably much easier to use string.replace then. Also this isn't part of any spec (if ES 4 adds this parameter its easier to implement it then, rather than now and confuse developers).

> 5.
> There also other useful methods
> http://msdn.microsoft.com/en-us/library/system.string_members.aspx
> like StartsWith/EndsWith, IndexOfAny, 
> PadLeft/PadRight (or better PadStart/PadEnd/PadCenter)

bug 76946 talks about StartsWith/EndsWith. As for the others, you can file bugs on them.

Comment 19

[Mike Kaplinskiy [:mikekap] (at school, no time)]

Attachment: ES 3 whitespace test for trim

Pretty much copied directly from the draft. Doesn't completely pass.

Comment 20

[Biju]

(In reply to comment #18)
> bug 76946 talks about StartsWith/EndsWith. As for the others, you can file bugs
> on them.

done, bug 446252

Comment 21

[Bob Clary [:bc] (inactive)]

we don't plan on segregating these additions by requiring version 1.9?

Comment 22

[Mike Kaplinskiy [:mikekap] (at school, no time)]

Attachment: Tests for trim(Left/Right) in patch form

I put this in ecma_3/String . Not sure how right that is.

Comment 23

[Bob Clary [:bc] (inactive)]

(In reply to comment #22)
> 
> I put this in ecma_3/String . Not sure how right that is.
> 

I'm not sure where it should go without guidance from brendan. It seems appropriate for js1_8 or possibly even js1_9 although if it is not guarded by a version check I guess it doesn't matter that much. Since it isn't part of the actual ecma3 spec, if it stays in ecma_3, it should go into ecma_3/extensions/. You do test for the existence of the functions though so engines that don't support it won't be penalized.

Your new test doesn't cover the characters in attachment 330263 [details] though. Should it?

Comment 24

[Mike Kaplinskiy [:mikekap] (at school, no time)]

Attachment: Space character class test (addition to 15.10.2.12.js)

(In reply to comment #23)
> Your new test doesn't cover the characters in attachment 330263 [details] though. Should
> it?
> 

Here it is, except I can't find how to flag a javascript test as a failure. The test fails for four of those characters:
FAILED! [reported from test()] Is OGHAM SPACE MARK a space : Expected value 'true', Actual value 'false'
FAILED! [reported from test()] Is MONGOLIAN VOWEL SEPARATOR a space : Expected value 'true', Actual value 'false'
FAILED! [reported from test()] Is NARROW NO-BREAK SPACE a space : Expected value 'true', Actual value 'false'
FAILED! [reported from test()] Is MEDIUM MATHEMATICAL SPACE a space : Expected value 'true', Actual value 'false' 

The definition of the \s class of in 15.10.2.12 of ecma 3, so it should be in the right place.

The test also doesn't use trim anymore, just regular expressions.

Should I add a combined diff for both tests, or is it easier to keep them separate?

Comment 25

[Bob Clary [:bc] (inactive)]

Either separate or combined diffs are ok with me. 

For the modification to ecma_3/RegExp/15.10.2.12.js, you are using a js1_6 feature |forEach| which is not definitely not part of ecma3. Please just use normal ecma3 array methods if you want to modify the test in ecma_3/RegExp/.

I'll deal with recording the known failures for the various brances when we check the tests in.

It would be good to also modify the trim, leftTrim, rightTrim tests to use these additional whitespace characters.

If you want, I'll do the tests and get your review. That might be simplest.

Comment 26

[Brendan Eich [:brendan]]

(In reply to comment #22)
> Created an attachment (id=332598) [details]
> Tests for trim(Left/Right) in patch form
> 
> I put this in ecma_3/String . Not sure how right that is.

Better to use ecma_3.1 or whatever the filename convention allows (ecma_3_1), but trimLeft and trimRight are not in ES3.1 as drafted. So we should use js1_8_1 (the 1 runs up against js there, unlike 3 which is separated by a _ from ecma). Yes, I'm officially in favor of using 1.8.1 as the js version for what's going into Gecko (mozilla milestone) 1.9.1 and Firefox 3.1.

/be

Comment 27

[Mike Kaplinskiy [:mikekap] (at school, no time)]

Attachment: Space character class test (addition to 15.10.2.12.js) v2

Here's the fix for forEach.

(In reply to comment #26)
> Better to use ecma_3.1 or whatever the filename convention allows (ecma_3_1),
> but trimLeft and trimRight are not in ES3.1 as drafted. So we should use
> js1_8_1 (the 1 runs up against js there, unlike 3 which is separated by a _
> from ecma). Yes, I'm officially in favor of using 1.8.1 as the js version for
> what's going into Gecko (mozilla milestone) 1.9.1 and Firefox 3.1.

Are you suggesting splitting the trim testcase into two files?

(In reply to comment #25)
> If you want, I'll do the tests and get your review. That might be simplest.

That would be great. Thanks.

Comment 28

[Brendan Eich [:brendan]]

(In reply to comment #27)
> Are you suggesting splitting the trim testcase into two files?

No, I meant what I wrote. Until ES3.1 is an Ecma standard (bound for ISO, we hope) the tests can all go under a js1_* directory. We can move trim's later.

/be

Comment 29

[Bob Clary [:bc] (inactive)]

Attachment: ecma_3_1_js1_8_1.tar.bz2

Here are the tests organized into ecma_3_1 and js1_8_1 in a tarball. Untar it into js/tests. These assume the tests in those directories will be run with version 1.8 for now. Mike, let me know if these work ok for you.

Brendan, if this is ok for moving forward let me know and I'll check them in along with the necessary changes to sisyphus to support the new directories.

Brendan, will we have an actual version 1.8.1 that we can select with the mimetype text/javascript;version=1.8.1 for the browser and version(181) in the shell?

Comment 30

[Mike Kaplinskiy [:mikekap] (at school, no time)]

Comment on attachment 332891 [details]
ecma_3_1_js1_8_1.tar.bz2

This looks good, just one small (nit-ish) change.

js/tests/js1_8_1/String/regress-305064.js:
+var summary = 'Tests the trim, trimRight  and trimLeft methods';
+
+printBugNumber(BUGNUMBER);

You may as well declare actual, expected and str here (as the other tests do).

Super-nit:
js/tests/ecma_3_1/RegExp/regress-305064.js:
+  };
No ;

r=me, except I don't have the editbugs priv to set r+.

With bug 412296, the patch is obsolete so I'll attach a new one in a sec.

Comment 31

[Mike Kaplinskiy [:mikekap] (at school, no time)]

Attachment: Patch against current trunk

Comment 32

[Biju]

(In reply to comment #18)
> > 3.
> > .Net trim/trimStart/trimEnd  also has a feature to take a char array as
> > parameter to remove other char
> > http://msdn.microsoft.com/en-us/library/system.string.trim.aspx
> > it will be better if we add that feature too.
> 
> Probably much easier to use string.replace then. Also this isn't part of any
> spec (if ES 4 adds this parameter its easier to implement it then, rather than
> now and confuse developers).

What is the decision on trimStart/trimEnd ?

Comment 33

[Bob Clary [:bc] (inactive)]

(In reply to comment #30)
> (From update of attachment 332891 [details])

I missed this on the original check in. 

/cvsroot/mozilla/js/tests/ecma_3_1/RegExp/regress-305064.js,v  <--  regress-305064.js
new revision: 1.2; previous revision: 1.1

/cvsroot/mozilla/js/tests/js1_8_1/String/regress-305064.js,v  <--  regress-305064.js
new revision: 1.2; previous revision: 1.1

Comment 34

[Bob Clary [:bc] (inactive)]

mozilla-central: changeset:   16621:fcb7767e828d for the tests.

Comment 35

[Bob Clary [:bc] (inactive)]

Dão, I just checked in the test, not the patch. Did you check in the patch? If not, the checkin-needed should be reinstated.

Comment 36

[Dão Gottwald [:dao]]

The patch has a pending superreview.

Comment 37

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #30)
> r=me, except I don't have the editbugs priv to set r+.

I've granted you editbugs+canconfirm.

Comment 38

[Brendan Eich [:brendan]]

Comment on attachment 332985 [details] [diff] [review]
Patch against current trunk

Thanks, this looks fine -- I'll tweak (alphabetize where the three names are adjacent: Left before Right) and land.

/be

Comment 39

[Brendan Eich [:brendan]]

Landed in m-c:

http://hg.mozilla.org/mozilla-central/rev/99a1bd41d3dd

/be

Comment 40

[Bob Clary [:bc] (inactive)]

filed 457422 on the undetected spaces.

Comment 41

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2 modulo bug 457422