Closed Bug 305064 Opened 19 years ago Closed 16 years ago

Add trim, trimLeft, and trimRight features for javascript strings

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

VERIFIED FIXED
mozilla1.9.1b1

People

(Reporter: egonknapen, Assigned: mikekap)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, verified1.9.1)

Attachments

(2 files, 9 obsolete files)

3.45 KB, application/x-bzip2
Details
3.00 KB, patch
brendan
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → general
Component: General → JavaScript Engine
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → Trunk
Shaver, should I use wiki.mozilla.org or developer.mozilla.org to catalog,
judge, modify, and select or reject new native methods?

/be
Status: UNCONFIRMED → NEW
Ever confirmed: true
wiki to design, devmo to document.
Summary: implementing a few extra feature for Strings in javascript nativ → Add trim, ltrim, and rtrim features for javascript strings
Blocks: 220348
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
Just .trim() would be enough, any .trim() would be fine... I personally never use the ltrim or rtrim functions either, only trim()
Adding this to the 1.9.1 triage queue by marking this wanted1.9.1?.  If this should be a blocker, please mark accordingly.
Flags: wanted1.9.1?
Priority: -- → P3
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...
Flags: wanted1.9.1? → wanted1.9.1+
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attached patch Patch v2 (obsolete) — Splinter Review
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?
Attachment #329909 - Attachment is obsolete: true
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.
Attachment #330131 - Attachment is obsolete: true
Attachment #330145 - Flags: review?(igor)
Assignee: general → mike.kaplinskiy
(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 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.
Attachment #330145 - Flags: review?(igor) → review+
Mike, please attach a patch covering Igor's final nit, and we'll get this landed. Thanks!
Attached patch Patch v4 (obsolete) — Splinter Review
Attachment #330145 - Attachment is obsolete: true
Attachment #330155 - Flags: review?
Attachment #330155 - Flags: review? → review?(igor)
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)
Attachment #330155 - Flags: review?(igor) → review+
(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.
Attached file Test case for trim (obsolete) —
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.
Attached file ES 3 whitespace test for trim (obsolete) —
Pretty much copied directly from the draft. Doesn't completely pass.
Whiteboard: checkin-needed
Blocks: 446252
(In reply to comment #18)
> bug 76946 talks about StartsWith/EndsWith. As for the others, you can file bugs
> on them.

done, bug 446252

we don't plan on segregating these additions by requiring version 1.9?
I put this in ecma_3/String . Not sure how right that is.
(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?
(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?
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.
(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
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.
Attachment #332673 - Attachment is obsolete: true
(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
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?
Attachment #330258 - Attachment is obsolete: true
Attachment #330263 - Attachment is obsolete: true
Attachment #332598 - Attachment is obsolete: true
Attachment #332678 - Attachment is obsolete: true
Attachment #332891 - Flags: review?(mike.kaplinskiy)
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.
Attachment #330155 - Attachment is obsolete: true
Attachment #332985 - Flags: superreview?(brendan)
(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 ?
(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
Flags: in-testsuite+
Flags: in-litmus-
mozilla-central: changeset:   16621:fcb7767e828d for the tests.
Whiteboard: checkin-needed
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.
The patch has a pending superreview.
(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 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
Attachment #332985 - Flags: superreview?(brendan) → superreview+
Attachment #332985 - Flags: superreview+ → review+
Landed in m-c:

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

/be
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed
Summary: Add trim, ltrim, and rtrim features for javascript strings → Add trim, trimLeft, and trimRight features for javascript strings
filed 457422 on the undetected spaces.
Blocks: es5
Blocks: 472393
Blocks: 472395
Keywords: verifyme
Target Milestone: --- → mozilla1.9.1b1
v 1.9.1, 1.9.2 modulo bug 457422
Status: RESOLVED → VERIFIED
Keywords: verifymeverified1.9.1
Depends on: 506563
Blocks: 506563
No longer depends on: 506563
Attachment #332891 - Flags: review?(mike.kaplinskiy)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: