Closed
Bug 305064
Opened 18 years ago
Closed 15 years ago
Add trim, trimLeft, and trimRight features for javascript strings
Categories
(Core :: JavaScript Engine, enhancement, P3)
Core
JavaScript Engine
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.
Updated•18 years ago
|
Assignee: nobody → general
Component: General → JavaScript Engine
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → Trunk
Comment 1•18 years ago
|
||
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.
Updated•18 years ago
|
Summary: implementing a few extra feature for Strings in javascript nativ → Add trim, ltrim, and rtrim features for javascript strings
Comment 3•15 years ago
|
||
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
Reporter | ||
Comment 4•15 years ago
|
||
Just .trim() would be enough, any .trim() would be fine... I personally never use the ltrim or rtrim functions either, only trim()
Comment 5•15 years ago
|
||
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
Reporter | ||
Comment 6•15 years ago
|
||
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...
Updated•15 years ago
|
Flags: wanted1.9.1? → wanted1.9.1+
Assignee | ||
Comment 7•15 years ago
|
||
Comment 8•15 years ago
|
||
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.
Assignee | ||
Comment 9•15 years ago
|
||
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 10•15 years ago
|
||
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.
Assignee | ||
Comment 11•15 years ago
|
||
Attachment #330131 -
Attachment is obsolete: true
Updated•15 years ago
|
Attachment #330145 -
Flags: review?(igor)
Updated•15 years ago
|
Assignee: general → mike.kaplinskiy
Comment 12•15 years ago
|
||
(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•15 years ago
|
||
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+
Comment 14•15 years ago
|
||
Mike, please attach a patch covering Igor's final nit, and we'll get this landed. Thanks!
Assignee | ||
Comment 15•15 years ago
|
||
Attachment #330145 -
Attachment is obsolete: true
Attachment #330155 -
Flags: review?
Assignee | ||
Updated•15 years ago
|
Attachment #330155 -
Flags: review? → review?(igor)
Comment 16•15 years ago
|
||
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)
Updated•15 years ago
|
Attachment #330155 -
Flags: review?(igor) → review+
Comment 17•15 years ago
|
||
(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.
Assignee | ||
Comment 18•15 years ago
|
||
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.
Assignee | ||
Comment 19•15 years ago
|
||
Pretty much copied directly from the draft. Doesn't completely pass.
Assignee | ||
Updated•15 years ago
|
Whiteboard: checkin-needed
Comment 20•15 years ago
|
||
(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•15 years ago
|
||
we don't plan on segregating these additions by requiring version 1.9?
Assignee | ||
Comment 22•15 years ago
|
||
I put this in ecma_3/String . Not sure how right that is.
Comment 23•15 years ago
|
||
(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?
Assignee | ||
Comment 24•15 years ago
|
||
(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•15 years ago
|
||
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•15 years ago
|
||
(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
Assignee | ||
Comment 27•15 years ago
|
||
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
Comment 28•15 years ago
|
||
(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•15 years ago
|
||
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)
Assignee | ||
Comment 30•15 years ago
|
||
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.
Assignee | ||
Comment 31•15 years ago
|
||
Attachment #330155 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Attachment #332985 -
Flags: superreview?(brendan)
Comment 32•15 years ago
|
||
(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•15 years ago
|
||
(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-
Comment 34•15 years ago
|
||
mozilla-central: changeset: 16621:fcb7767e828d for the tests.
Updated•15 years ago
|
Whiteboard: checkin-needed
Comment 35•15 years ago
|
||
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•15 years ago
|
||
The patch has a pending superreview.
Comment 37•15 years ago
|
||
(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•15 years ago
|
||
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+
Updated•15 years ago
|
Attachment #332985 -
Flags: superreview+ → review+
Comment 39•15 years ago
|
||
Landed in m-c: http://hg.mozilla.org/mozilla-central/rev/99a1bd41d3dd /be
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
Keywords: dev-doc-needed
Updated•15 years ago
|
Summary: Add trim, ltrim, and rtrim features for javascript strings → Add trim, trimLeft, and trimRight features for javascript strings
Comment 40•15 years ago
|
||
filed 457422 on the undetected spaces.
Updated•15 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Comment 41•14 years ago
|
||
v 1.9.1, 1.9.2 modulo bug 457422
Status: RESOLVED → VERIFIED
Keywords: verifyme → verified1.9.1
Updated•14 years ago
|
Updated•11 years ago
|
Attachment #332891 -
Flags: review?(mike.kaplinskiy)
You need to log in
before you can comment on or make changes to this bug.
Description
•