Created attachment 213473 [details] [review]
modules: Import a new "format" module

Imported from gnome-shell, this provides printf-like formatting to
JavaScript.

Created attachment 213474 [details] [review]
Switch string formatting to the one inside gjs

Review of attachment 213473 [details] [review]:

Apart from this comment below, looks good to me.
Should we try to attach this to the String class directly from GJS? It would avoid the need to set up the override in every application (and applications could still override it again if they want it to do something different).

::: modules/format.c
@@ +41,3 @@
+        return JS_FALSE;
+    if (!gjs_string_from_utf8(context, g_strdup_printf("%Id", intval), -1, &rval))
+        return JS_FALSE;

Aren't you leaking the stirng returned by g_strdup_printf() here? It should be g_free()-d.

I don't think that modifying a prototype is something that an interpreter should do.

Created attachment 213616 [details] [review]
modules: Import a new "format" module

Imported from gnome-shell, this provides printf-like formatting to
JavaScript.



Gah, for some reason I always think string_from_utf8 steals its parameter.

(In reply to comment #4)
> I don't think that modifying a prototype is something that an interpreter
> should do.

But the format() function will stop working in case the application doesn't set it up, since doing "return vprintf(this, arguments);" assumes that format() is part of String.Prototype.
So my idea was to either we set it up automatically, or only provide vprintf() in Format and let applications define its interaction with String.Prototype.

(In reply to comment #6)
> (In reply to comment #4)
> > I don't think that modifying a prototype is something that an interpreter
> > should do.
> 
> But the format() function will stop working in case the application doesn't set
> it up, since doing "return vprintf(this, arguments);" assumes that format() is
> part of String.Prototype.
> So my idea was to either we set it up automatically, or only provide vprintf()
> in Format and let applications define its interaction with String.Prototype.

The goal was "here's a readymade method for the prototype, install it if you want to", so everybody wouldn't have to create their own format method, even if it's only one line of code.

I tend to agree with Cosimo here ... we should just set it up. It isn't part of the EMCAScript standard but we provide other non standard stuff anyway so not seeing any harm it would cause.

(In reply to comment #8)
> I tend to agree with Cosimo here ... we should just set it up. It isn't part of
> the EMCAScript standard but we provide other non standard stuff anyway so not
> seeing any harm it would cause.

We don't modify any other prototypes. We made Lang.bind instead of Function.prototype.bind. I don't think it's right to stop that process now.

(In reply to comment #9)
> (In reply to comment #8)
> > I tend to agree with Cosimo here ... we should just set it up. It isn't part of
> > the EMCAScript standard but we provide other non standard stuff anyway so not
> > seeing any harm it would cause.
> 
> We don't modify any other prototypes. We made Lang.bind instead of
> Function.prototype.bind. I don't think it's right to stop that process now.

OK, makes sense.

Ping?

Patch looks fine to me, but I'm not a GJS maintainer...

Review of attachment 213616 [details] [review]:

Looks good.

Review of attachment 213474 [details] [review]:

Looks good.

Attachment 213616 [details] pushed as 2ef9fe8 - modules: Import a new "format" module

Comment on attachment 213474 [details] [review]
Switch string formatting to the one inside gjs

Attachment 213474 [details] pushed as 8a9e3e0 - Switch string formatting to the one inside gjs