Created attachment 143488 [details] [review]
Add String formatting

Created attachment 143626 [details] [review]
Add String formatting

Fixed gjs warnings.

Hm... it's nice and simple, but maybe too simple? I feel like it should either implement *just* %s, or else it needs to implement all of printf, or else people will end up tripping over it.

And actually, for proper translation of strings, don't you need the ability to reorder the arguments? ("%$2s")

Maybe a simple version of the .NET String.format would work better ("{0}" gets replaced with the first argument, "{1}" with the second, etc. In the real .NET version you can specify field widths and conversions and stuff too, but we can just ignore that part.)

If we're keeping printf-like, it needs to handle "%%".

(In reply to comment #3)
> Hm... it's nice and simple, but maybe too simple? I feel like it should either
> implement *just* %s, or else it needs to implement all of printf, or else
> people will end up tripping over it.
> 
> And actually, for proper translation of strings, don't you need the ability to
> reorder the arguments? ("%$2s")
> 
> Maybe a simple version of the .NET String.format would work better ("{0}" gets
> replaced with the first argument, "{1}" with the second, etc. In the real .NET
> version you can specify field widths and conversions and stuff too, but we can
> just ignore that part.)
> 
> If we're keeping printf-like, it needs to handle "%%".

I am fine either way,  but which one do we want? The .NET style is easier to implement but are translators used to {0}, {1} etc. instead of %s and friends?

I think what I said on IRC probably holds - if we have translated strings in the C code with %s style formatting, we don't want to have a mix in our po files.

We currently do have two tranlated format strings in the C code, though our goal long term is not to have much UI written in C. So I think that means going with the % style formatting.

I don't see the reordering as a blocker, though it wouldn't be hard to implement. We can wait until someone complains.

I would like %d/%f to fail on non-numeric strings 

(%d should convert floating point numbers to integer - round? truncate? I think probably truncate. Really it should behave however gjs behaves when you pass a floating point number to a function expecting an integer.)

On the other hand, %s should do toString() for everything - it shouldn't fail even if it's passed an object or null/undefined.

Agree that %% is a must.

Unknown format characters should be an error and not pass through to the output.

Created attachment 143759 [details] [review]
Add String formatting

New patch adds %% support and other fixes:

*) always use toString for %s
*) just use NaN for invalid %f, %d as discussed on IRC
*) truncate floating point numbers when passed to %d

Created attachment 143769 [details] [review]
Add String formatting

Created attachment 143771 [details] [review]
Add String formatting

Comment on attachment 143771 [details] [review]
Add String formatting

>+	let regex = /[^%]%{1}(([sdf])|(\.([0-9]+)(f)))/g;

this could be a module-level const rather than a function-level let.

"%{1}" ought to mean exactly the same thing as "%", shouldn't it? Also, no need for the ()s around the f.

>+	let i = 0;
>+        let match;

you have tabs in some places and spaces in others. (should be spaces everywhere)

>+	    if (!(/^\./.test(match[1]))) {

That seems quite opaque. How about: if (match[2] != "") or something like that

>+	    	    case 's':
>+	    	    	str = str.replace(new RegExp("([^%])%{1}" + match[1]), "$1" + arguments[i].toString());

Inelegant, and I don't think it does the right thing with %% anyway. Did you test this? Here are some test cases:

"%s".format("foo") => "foo"
"%%s".format("foo") => "%s"
"%%%s".format("foo") => "%foo"
"%s".format("%s", "bar") => "%s"

Using just a "replace leftmost %-thing" loop won't work; you need to keep track of what part of the string you have already substituted into, and only do new replacements after that point.

>+	    	str = str.replace(new RegExp("([^%])%{1}" + match[3]), "$1" + parseFloat(arguments[i]).toFixed(match[4]));

does match[4] need a parseInt() around it?

>+++ b/js/ui/main.js
>+const Format = imports.misc.format;

Hm... not sure I like the idea that just importing the file has a side effect. Might be better if main.js had to call some Format method to do that. Or Main.start() could do "String.format = Format.format" itself?

It would be nice to have some test cases. Owen is working on a test framework now. I wonder if there isn't some way we could do something like the python 'if __name__ == "__main__"' hack so you could just do "gjs js/ui/format.js" to run the tests?

(In reply to comment #9)
> >+	    	    case 's':
> >+	    	    	str = str.replace(new RegExp("([^%])%{1}" + match[1]), "$1" + arguments[i].toString());
> 
> Inelegant, and I don't think it does the right thing with %% anyway. Did you
> test this? Here are some test cases:
> 
> "%s".format("foo") => "foo"
> "%%s".format("foo") => "%s"
> "%%%s".format("foo") => "%foo"
> "%s".format("%s", "bar") => "%s"
> 
> Using just a "replace leftmost %-thing" loop won't work; you need to keep track
> of what part of the string you have already substituted into, and only do new
> replacements after that point.

Probably what you want to do is use the ability to pass a function into String.replace()

 return str.replace(<regexp>, 
                    function(matched, group1, group2, ...) {
     if (group1 == ...) {
        return ...;
     } else {
        ....
     }
 });

https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Global_Objects/String/replace#Specifying_a_function_as_a_parameter

Created attachment 143884 [details] [review]
Add String formatting

New patch:

*) regex is now a module level const
*) moved the String.prototype.format part to Main.start()
*) Fixed %% handling (passes the test cases now)

Review of attachment 143884 [details] [review]:

::: /dev/null
@@ +3,3 @@
+const regex = /([%]+)(([sdf])|(\.([0-9]+)f))/g;
+
+function format() {

Needs a comment here describing what this function does and how it is used

@@ +4,3 @@
+
+function format() {
+	let str = this;

Identation - we need 4 space idents with spaces, you have single-tab idents.

@@ +7,3 @@
+	let i = 0;
+	let args = arguments;
+	str = str.replace(regex, function (str, m1, m2, m3, m4, m5, offset, s) {

I disagree with Dan here I think it's much clearer if the regular expression is inline

 return str.replace(/([%]+)(([sdf])|(\.([0-9]+)f))/g,
                    function(str, percentGroup, genericGroup, floatWithPercisionGroup) {

Because then you can easily match the groups variables up with the groups in the regular expression. There's no reason to include the 'offset' and 's' args since you don't need them - extra arguments will just be ignored. Using meaningful names will make the code more readable.

Rather than using [%]+ here (which is the same as %+), do it as

 /%(?:(%)|([sdf])|(\.([0-9]+)f)/

Since %%%s will be gotten wrong otherwise. The non-capturing (?:) match avoids having to have a dummy group variable.

But this still needs some adjustment since it won't get what I was talking about right - it needs to fail for unknown format characters. So you really want something more like:

 /%(?:(?:\.([0-9]+))?(.)/

And then do an explicit:
 
 if (precisionGroup && formatCharacter != 'f') {
     throw new Error("precision can only be specified for 'f'");

@@ +36,3 @@
+				});
+	return str;
+}

As Dan said, this really needs tests. What I'd do for now is to create a separate test script that can be run with:

 gjs-console -I . testformat.js

And put it right aloong side format.js in your patch, and then we can sort it out later where it should actually live. I don't know a good way of doing inline tests python-style for Javascript. I suppose one approach would be to just have  a runTests() function with a standard name.

(In reply to comment #12)
> Review of attachment 143884 [details] [review]:
> 
> ::: /dev/null
> @@ +3,3 @@
> +const regex = /([%]+)(([sdf])|(\.([0-9]+)f))/g;
> +
> +function format() {
> 
> Needs a comment here describing what this function does and how it is used

OK.

> @@ +4,3 @@
> +
> +function format() {
> +    let str = this;
> 
> Identation - we need 4 space idents with spaces, you have single-tab idents.

OK, will switch to 4 spaces.

> @@ +7,3 @@
> +    let i = 0;
> +    let args = arguments;
> +    str = str.replace(regex, function (str, m1, m2, m3, m4, m5, offset, s) {
> 
> I disagree with Dan here I think it's much clearer if the regular expression is
> inline

OK

>  return str.replace(/([%]+)(([sdf])|(\.([0-9]+)f))/g,
>                     function(str, percentGroup, genericGroup,
> floatWithPercisionGroup) {
> 
> Because then you can easily match the groups variables up with the groups in
> the regular expression. There's no reason to include the 'offset' and 's' args
> since you don't need them - extra arguments will just be ignored. Using
> meaningful names will make the code more readable.

Yeah will remove them.

> Rather than using [%]+ here (which is the same as %+), do it as
> 
>  /%(?:(%)|([sdf])|(\.([0-9]+)f)/
> 
> Since %%%s will be gotten wrong otherwise. The non-capturing (?:) match avoids
> having to have a dummy group variable.

OK

> But this still needs some adjustment since it won't get what I was talking
> about right - it needs to fail for unknown format characters. So you really
> want something more like:
> 
>  /%(?:(?:\.([0-9]+))?(.)/

Shouldn't the last group be limited to s,d and f ?

> And then do an explicit:
> 
>  if (precisionGroup && formatCharacter != 'f') {
>      throw new Error("precision can only be specified for 'f'");

OK.

> @@ +36,3 @@
> +                });
> +    return str;
> +}
> 
> As Dan said, this really needs tests. What I'd do for now is to create a
> separate test script that can be run with:
> 
>  gjs-console -I . testformat.js
> 
> And put it right aloong side format.js in your patch, and then we can sort it
> out later where it should actually live. I don't know a good way of doing
> inline tests python-style for Javascript. I suppose one approach would be to
> just have  a runTests() function with a standard name.

OK, will try to come up with some testscases.

(In reply to comment #13)
> > But this still needs some adjustment since it won't get what I was talking
> > about right - it needs to fail for unknown format characters. So you really
> > want something more like:
> > 
> >  /%(?:(?:\.([0-9]+))?(.)/
> 
> Shouldn't the last group be limited to s,d and f ?

No - that's the point - if the input has %u it needs to be captured so you can throw an error. If you limited the last group to sdf then you'd just skip anything that didn't match.

Created attachment 144430 [details] [review]
Add String formatting

Fixed up patch:

*) Regex now inline
*) Better error handling
*) Adds testcases
*) Style cleanups

Created attachment 144459 [details] [review]
Add String formatting

*) Remove trailing whitespace

Review of attachment 144459 [details] [review]:

Looking good, a few more comments:

::: js/misc/format.js
@@ +15,3 @@
+    let i = 0;
+    let args = arguments;
+    return str.replace(/(%+)(?:\.([0-9]))?(.)/g, function (str, percentGroup, precisionGroup, genericGroup) {

I think you want [0-9]+ for the precision group

@@ +19,3 @@
+                    if (percentGroup.length != 1) {
+                        if ((percentGroup.length % 2) == 0) {
+                            return str.replace(/%%/g, '%');

You don't want to do percents this way any more - just add case '%' to the switch and drop the (%+). Then things will simplify a whole lot.

@@ +44,3 @@
+                            break;
+                        default:
+                            throw new Error('Invalid format %' + genericGroup);

I'd probably say "Invalid conversion character: %" - the format is the whole string. And maybe 'Unknown' or 'Unsupported' rather than 'Invalid'

::: js/misc/testformat.js
@@ +4,3 @@
+ * Test cases for the Format module
+ * Run with:
+ * GJS_DEBUG_OUTPUT='stderr' gjs-console -I . testformat.js

OK, better suggestion for this (now that we have a tests/ directory) is to put this as tests/unit/format.js (and add it to tests/Makefile.am:TEST_JS). Start it with:

===
const JsUnit = imports.jsUnit;
const assertEquals = JsUnit.assertEquals;
const assertRaises = JsUnit.assertRaises;
===

(There's more functions in JsUnit - assertNull, assertTrue, assertNan, etc, but those are the immediately useful ones)

@@ +10,3 @@
+String.prototype.format = Format.format;
+
+log("%s".format('foo'));

this would then be assertEquals("foo", "%s".format("foo"))

@@ +19,3 @@
+//should throw an Exception:
+/*
+log("%.2d".format(5.21));

and this would be:

// Precision not allowed for %d
assertRaises(function() { "%.2d".format(5.21); });

::: js/ui/main.js
@@ +55,3 @@
 
+    // Enable String formatting
+    String.prototype.format = Format.format;

Probably should go into Environment.js - which is things that are shared between the application and the test cases.

Created attachment 144731 [details] [review]
Add String formatting

Fixed up patch

Created attachment 144732 [details] [review]
Add String formatting

Fix comment

Review of attachment 144732 [details] [review]:

Very, very, close.

::: js/misc/format.js
@@ +17,3 @@
+
+    return str.replace(/%(?:\.([0-9]+))?(.)/g, function (str, precisionGroup, genericGroup) {
+                    let result = "";

I think you can just return in all cases rather than having the result temporary.

::: tests/Makefile.am
@@ +12,3 @@
 	testcommon/border-image.png		\
+	testcommon/ui.js                \
+	testcommon/testformat.js

testformat/js shouldn't be under testcommon (which is utilities for tests to use) but under a new subdirectory called unit. And it should be called 'format.js' not 'testformat.js'. It's under tests/ so it's a test, we don't need to say that in every filename.

[ There's an argument that it's a little confusing to have two calendar.js, format.js, etc, in the tree, but I already went that way for calendar.js so you need match it :) ]

::: tests/testcommon/testformat.js
@@ +12,3 @@
+
+// Set up the String class here again to work around 
+// gjs multiple context handling

There isn't enough detail here for this to be a useful comment. You could not have a comment at all to keep it clean, or you could expand, and say

 // We can't depend on environment.js to set up the String.prototype.format, because 
 // the tests  run in one JS context, and the imports run in the GJS "load context" which
 // has its own copy of the String class

Comments should be written as if you are going to read them in 3 years, and will have no memory of when you wrote them.

@@ +17,3 @@
+String.prototype.format = Format.format;
+
+Environment.init();

Though we don't have a style yet for our unit tests - this is the first one! - I think since this is a test of non-UI functionality (misc/format.js, not ui/format.js) it probably shouldn't initialization ui.Environment in any case. (Though the comment above is still useful in case someone is tempted to "fix it" later to use environment.js and is puzzled about what is going on.)

(In reply to comment #20)
> Review of attachment 144732 [details] [review]:
> 
> Very, very, close.
> 
> ::: js/misc/format.js
> @@ +17,3 @@
> +
> +    return str.replace(/%(?:\.([0-9]+))?(.)/g, function (str, precisionGroup,
> genericGroup) {
> +                    let result = "";
> 
> I think you can just return in all cases rather than having the result
> temporary.

Yeah should have noticed that while removing the "+="

> ::: tests/Makefile.am
> @@ +12,3 @@
>      testcommon/border-image.png        \
> +    testcommon/ui.js                \
> +    testcommon/testformat.js
> 
> testformat/js shouldn't be under testcommon (which is utilities for tests to
> use) but under a new subdirectory called unit. And it should be called
> 'format.js' not 'testformat.js'. It's under tests/ so it's a test, we don't
> need to say that in every filename.

OK

> [ There's an argument that it's a little confusing to have two calendar.js,
> format.js, etc, in the tree, but I already went that way for calendar.js so you
> need match it :) ]

OK ;)

> ::: tests/testcommon/testformat.js
> @@ +12,3 @@
> +
> +// Set up the String class here again to work around 
> +// gjs multiple context handling
> 
> There isn't enough detail here for this to be a useful comment. You could not
> have a comment at all to keep it clean, or you could expand, and say
> 
>  // We can't depend on environment.js to set up the String.prototype.format,
> because 
>  // the tests  run in one JS context, and the imports run in the GJS "load
> context" which
>  // has its own copy of the String class
> 
> Comments should be written as if you are going to read them in 3 years, and
> will have no memory of when you wrote them.

OK, fixed that.

> @@ +17,3 @@
> +String.prototype.format = Format.format;
> +
> +Environment.init();
> 
> Though we don't have a style yet for our unit tests - this is the first one! -
> I think since this is a test of non-UI functionality (misc/format.js, not
> ui/format.js) it probably shouldn't initialization ui.Environment in any case.
> (Though the comment above is still useful in case someone is tempted to "fix
> it" later to use environment.js and is puzzled about what is going on.)

OK, removed "Environment.init();"

Created attachment 144740 [details] [review]
Add String formatting

Review of attachment 144740 [details] [review]:

Looks great. Two tiny fixes, but you don't need to reattach - just go ahead and push once you have them fixed.

::: tests/Makefile.am
@@ +11,3 @@
 	interactive/table.js			\
 	testcommon/border-image.png		\
+	testcommon/ui.js                \

\ needs to be lined up. (If you are working with non-8-space-tabs you may not notice that it isn't)

::: tests/unit/format.js
@@ +5,3 @@
+ */
+
+const Environment = imports.ui.environment;

This should be removed as well.

Comment on attachment 144740 [details] [review]
Add String formatting

Committed as 64cd51667dc498412cbaeb53cd92cc6de7a04e23 with the fixes mentioned in comment 23 and whitespace fixes.

Review of attachment 144740 [details] [review]:

Something

Created attachment 144796 [details] [review]
String formatting: Fix warning

Fix to not trigger a warning like:

JS ERROR: !!!   WARNING: 'anonymous function does not always return a value'

by adding a return ""; at the end of the anonymous function.

Review of attachment 144796 [details] [review]:

Looks good.

::: js/misc/format.js
@@ +42,3 @@
 
+                    return "";
+

Excess blank line (was there before, but shouldn't have been), and since it's there only to suppress the warning (seems like SpiderMonkey is failing to note the throw) you should comment that

 return ""; // Suppress warning

Comment on attachment 144796 [details] [review]
String formatting: Fix warning

Commited as edb50d5dc78b70df666ce962ae7924c3a3fd2151 with changes from comment 27