[Glass] MAStringReader>>visitNumberDescription broken?

[Dale Henrichs via Glass]

On 09/14/2015 02:54 AM, Iwan Vosloo via Glass wrote:
> On 09/09/2015 20:30, Dale Henrichs via Glass wrote:
>> From your information below, it does sound like having NumberParser 
>> use SqNumberParser as a default class for all of the class-side 
>> methods would likely solve the problem ... in Pharo3.0 the concrete 
>> class is called NumberParser and in GsDevKit the concrete clas is 
>> SqNumberParser and likely at some point in the past the Pharo folks 
>> collapsed the two classes, so from a compatibility perspective 
>> treating NumberParser as a concrete class is desired behavior in 
>> general...
>
> Dale, another question.
>
> I see that parse: and parse:onError: are defined on both NumberParser 
> and SqNumberParser (and the implementations on the different classes 
> are identical).
> These use (self new) which I would have thought one usually would do.
The history of these two classes is lost in the sands of time:)

Looking at a Pharo3.0 image, it looks like NumberParser has become a 
concrete class, so we could imagine moving all of the methods up to 
NumberParser (assuming no meaningful conflicts) and leaving 
SqNumberParser as an empty shell for compatibility reasons ...

I seem to recall that Mariano uses ExtendedNumberParser as his `default` 
parser class (I don't see an issue on this... hint, hint) so it would 
probably make sense to add the notion of a default parser class to 
NumberParser so that Mariano can choose his own flavor or even introduce 
one of his own ...
>
> With "use SqNumberParser as a default class for all of the class-side 
> methods", do you mean to  change parse:, parse:onError: and on: on 
> NumberParser _itself_ to hard-code the use of SqNumberParser (ie, your 
> "as default"), but lower down in the hierarchy the use of self is kept?

I was thinking along the lines I mentioned above where we'd have a 
defaultParserClass in NumberParser that would default to SqNumberParser 
(unless we move the methods up - then NumberParser) ...

GemStone has changed the standard print format produced by the base 
image in 3.3 (or late 3.2.x) so being able to customize the NumberParser 
is probably a good thing ...
>
> Anyways - if I do understand you correctly - this does solve the 
> problem without a change to NumberParser class>>isNumber: (which is 
> Magritte code itself)

Okay so really we are suffering from NumberParser being an abstract 
class in GemStone and as discussed above we have a couple of approaches 
to fixing the bug ...

If you submit a pull request that introduces the notion of a 
defaultParserClass (using a DefaultParserClass class variable with 
getter and setter methods) and change the class-side methods to use the 
defaultParserClass method (i.e., change `self new` to `self 
defaultParserClass new`) and make the defaultParserClass default to 
SqNumberParserClass, then I assume that is the minimum to address the 
Magritte issue ...

Moving the methods up to NumberParser and making it a concrete class can 
be left for another day and another bugfix sweep:)

Thanks,

Dale