MAStringReader>>visitNumberDescription broken?

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

_______________________________________________
Glass mailing list
[hidden email]
http://lists.gemtalksystems.com/mailman/listinfo/glass