dolphinsmalltalk / Dolphin

Dolphin Smalltalk Core Image
MIT License
303 stars 58 forks source link

Binary selector for expandMacrosWith... #1053

Closed rko281 closed 3 years ago

rko281 commented 3 years ago

When revisiting code I'm gradually replacing string concatenation with expandMacrosWith: and its variants. One (very) small annoyance is the need to wrap the modified String expression with parentheses, for example:

MessageBox warning: 'Could not locate ', anObject displayString, '. Please check and retry.'

...becomes

MessageBox warning: ('Could not locate <1d>. Please check and retry.' expandMacrosWith: anObject) 

I've implemented a simple workaround for this by adding the binary selector << as an alias for expandMacrosWithArguments:. Thus the above example becomes:

MessageBox warning: 'Could not locate <1d>. Please check and retry.' << {anObject} 

At its most basic this just needs a single method adding to String

<< anArray

    ^self expandMacrosWithArguments: anArray

On checking the base image it can be seen that the majority (>50%) of uses of expandMacros... involve just a single argument. A more sophisticated implementation using double-dispatch could avoid the need to wrap a single argument in an Array whilst still supporting multiple arguments where necessary.

String method:

<< anObject

    ^anObject expandMacrosIn: self

Object method:

expandMacrosIn: aString

    ^aString expandMacrosWith: self

Collection method:

expandMacrosIn: aString

    ^aString expandMacrosWithArguments: self

The Object implementation also needs duplicating in String to avoid a single String argument being treated as a collection of Characters.

This then enables expressions like:

'<1d> already exists' << anObject
'<1d> already exists in <2d>' << {anObject. anObject parent}

A small downside with this approach is where a collection is intended as an individual argument rather than the collection of arguments. However this can be worked around by wrapping the collection in an Array, e.g.

MessageBox warning: 'Collection <1p> too small' << {#(1 2 3)}

I'd like to submit this as a potential base enhancement (I'm happy to implement this myself, with associated tests).

fxgallego commented 3 years ago

I like it. But I prefer `$[anObject] already exists in $[anObject parent]` 😄

blairmcg commented 3 years ago

Interpolated strings would certainly be nice (although they don't really address the localisation requirement), but I think this is a fine idea John. The only immediate concern I had was whether snaffling the << binary selector for this would be regretted in future if we thought of a better use, especially if that use involved defining it on Object. However, as #<< is already in use in Integer, it is already unavailable for global use. This doesn't seem to clash with any precedent elsewhere either. So it gets my vote.