MAStringReader>>visitNumberDescription broken?

Hello,

In the code we've got (Magritte3 3.3.1 loaded from http://smalltalkhub.com/mc/Magritte/Magritte3/main/),

MAStringReader>>visitNumberDescription contains:

 NumberParser isNumber: self contents.

NumberParser is abstract on GemStone, so this always raises an exception. The exception is a bit masked, since it is caught higher up the stack with the result that numbers typed into an input for an MANumberDescription are always rejected as failing validateKind: in Magritte.

I have checked version 3.3.2 and 3.5.0 in this repository as well and find the same issue there.

According to https://github.com/magritte-metamodel/magritte, Magritte should be installed from http://seaside.gemstone.com/ss/magritte.html on GemStone - yet that code looks quite old to me...

Are we using the correct repository for GemStone?
And if so.. is the issue tracker for such a GemStone specific issue the one on https://github.com/magritte-metamodel/magritte ?
... or am I missing something else?

Regards
- Iwan

-- 
Reahl, the Python only web framework: http://www.reahl.org

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

GLASS mailing list wrote

  NumberParser isNumber: self contents.

I made the change for Pharo because Magritte was rolling it's own number validation, and not doing as good a job, so I favored delegation. Of course, Magritte doesn't have a cross-platform CI AFAICT; maybe we can set Travis up? Although, I see the Pharo CI job [1] has been failing too, so maybe there is a dependency missing? I'll take a look asap...

[1] https://ci.inria.fr/pharo-contribution/job/Magritte3/PHARO=20,VERSION=development,VM=vm/710/

Hello,

In the code we've got (Magritte3 3.3.1 loaded from http://smalltalkhub.com/mc/Magritte/Magritte3/main/),

MAStringReader>>visitNumberDescription contains:

 NumberParser isNumber: self contents.

NumberParser is abstract on GemStone, so this always raises an exception. The exception is a bit masked, since it is caught higher up the stack with the result that numbers typed into an input for an MANumberDescription are always rejected as failing validateKind: in Magritte.

I have checked version 3.3.2 and 3.5.0 in this repository as well and find the same issue there.

According to https://github.com/magritte-metamodel/magritte, Magritte should be installed from http://seaside.gemstone.com/ss/magritte.html on GemStone - yet that code looks quite old to me...

Yes, that information is out of date. I have a page[1] on GsDevKitHome wher information about using Magritte _will_ be found, when the dev branch work is released and the Magritte page is updated, I will update the link on https://github.com/magritte-metamodel/magritte ...

[1] https://github.com/GsDevKit/gsDevKitHome/tree/dev/projects/magritte3

Are we using the correct repository for GemStone?

I've a small amount of work with Magritte and Magritte 3.2.0 is the version that I was working with and IIRC the tests all passed at that point in time... I see that Sean is weighing in on the discussion as well and he is likely the most recent person to work with Magritte and GemStone ...

And if so.. is the issue tracker for such a GemStone specific issue the one on https://github.com/magritte-metamodel/magritte ?
... or am I missing something else?

I would say that that is a good place for Magritte/GemStone issues ... I keep an eye on the Magritte mailing list and the github project, but I may not see issues in a timely fashion, so it is worth pinging me here with a link to the issue if you submit one there.

Dale

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

Sean,

I have a travis test for Magritte[1] that validates the Magritte
installation instructions and runs the tests....

[1] https://github.com/GsDevKit/gsDevKitHome/blob/dev/tests/magritteTests.sh

On 9/2/15 5:01 AM, Sean P. DeNigris via Glass wrote:
_______________________________________________
Glass mailing list
[hidden email]
http://lists.gemtalksystems.com/mailman/listinfo/glass

On 9/2/15 2:53 AM, Iwan Vosloo via Glass wrote:

According to https://github.com/magritte-metamodel/magritte, Magritte should be installed from http://seaside.gemstone.com/ss/magritte.html on GemStone - yet that code looks quite old to me...

Yes, that information is out of date. I have a page[1] on GsDevKitHome wher information about using Magritte _will_ be found, when the dev branch work is released and the Magritte page is updated, I will update the link on https://github.com/magritte-metamodel/magritte ...

[1] https://github.com/GsDevKit/gsDevKitHome/tree/dev/projects/magritte3

Hi Dale,

Which repository should one then _currently_ be using for Magritte on GemStone?

Regards
- Iwan

-- 
Reahl, the Python only web framework: http://www.reahl.org

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

It turns out that in the GemStone/GsDevKit version of NumberParser
(actually SqNumberParser), isNumber: is not implemented ... was there a
major rewrite of NumberParser in Pharo4.0 (it's not in Pharo3.0 either)
or is it a simple extension method?

I might be able to squeeze out some time and take a bigger look at this
one:)

Dale
On 9/2/15 5:01 AM, Sean P. DeNigris via Glass wrote:
_______________________________________________
Glass mailing list
[hidden email]
http://lists.gemtalksystems.com/mailman/listinfo/glass

On 07/09/2015 16:56, Dale Henrichs via Glass wrote:
> It turns out that in the GemStone/GsDevKit version of NumberParser
> (actually SqNumberParser), isNumber: is not implemented ... was there
> a major rewrite of NumberParser in Pharo4.0 (it's not in Pharo3.0
> either) or is it a simple extension method?
>
> I might be able to squeeze out some time and take a bigger look at
> this one:)

 From what I can see, Magritte-Model adds isNumber in the Pharo and
GemStone case. The real problem is perhaps how isNumber: is implemented
OR perhaps the implementation of parse:onError: which it sends.

Regards
- Iwan

--
Reahl, the Python only web framework: http://www.reahl.org

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

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

Iwan,

Okay, then I need to dive into the Magritte implementation directly and
try to figure things out from there ... I naively thought that the
problem was the direct use of NumberParser in the code, instead of
SqNumberParser - it would be relatively easy to change NumberParser to
redirect instance creation to SqNumberParser, but It sounds like that
isn't the source ofthe problem ...

BTW are Magritte tests failing on GemStone or is this an untested chunk
of code?

Dale

On 09/08/2015 02:50 AM, Iwan Vosloo via Glass wrote:
_______________________________________________
Glass mailing list
[hidden email]
http://lists.gemtalksystems.com/mailman/listinfo/glass

Mariano,

Could you submit an issue on this [1]? I would like a little more detail on the circumstances for needing to use ExtendedNumberParser (unconditionally) instead of SqNumberParser ... the other two cases, I think I understand ...

As always a pull request will be integrated into the release much quicker than waiting for me to get around to dealing with the issue:)

Dale

[1] https://github.com/GsDevKit/GsDevKit/issues

On Tue, Sep 8, 2015 at 6:50 AM, Iwan Vosloo via Glass <[hidden email]> wrote:

On 07/09/2015 16:56, Dale Henrichs via Glass wrote:

It turns out that in the GemStone/GsDevKit version of NumberParser (actually SqNumberParser), isNumber: is not implemented ... was there a major rewrite of NumberParser in Pharo4.0 (it's not in Pharo3.0 either) or is it a simple extension method?

I might be able to squeeze out some time and take a bigger look at this one:)

From what I can see, Magritte-Model adds isNumber in the Pharo and GemStone case. The real problem is perhaps how isNumber: is implemented OR perhaps the implementation of parse:onError: which it sends.

Hi guys, 

This is not 100% same topic, but wanted to say it in case we fix both. Unfortunately, I cannot remember what was broken, and I did not even wrote a comment....but as part of my sad "overrides" in GemStone, I have an override of Integer >> #readFrom: 

 

Integer >> readFrom: aStringOrStream 

"Answer a new Integer as described on the stream, aStream.

Embedded radix specifiers not allowed - use Number readFrom: for that."

^self readFrom: aStringOrStream base: 10

And I had to override it like this:

Integer >> faReadFrom: aStream ^ (SqNumberParser on: aStream) nextIntegerBase: 10

And I have to also change:

SqNumberParser class >> on: aStringOrStream

^ExtendedNumberParser new on: aStringOrStream

SqNumberParser >> exponentLetters

"answer the list of possible exponents for Numbers.

Note: this parser will not honour precision attached to the exponent.

different exponent do not lead to different precisions.

only IEEE 754 floating point numbers will be created"

^'edqEDQ'

Regards
- Iwan

--
Reahl, the Python only web framework: http://www.reahl.org

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

--

Mariano
http://marianopeck.wordpress.com

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

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

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

The issues for glassdb were physically moved to GsDevKit/GsDevKit ... the issue numbers were preserved, so this is probably the url (droid?) you are looking for:

  https://github.com/GsDevKit/GsDevKit/issues/20

Dale

On Tue, Sep 8, 2015 at 4:56 PM, Dale Henrichs via Glass <[hidden email]> wrote:

Mariano,

Could you submit an issue on this [1]? I would like a little more detail on the circumstances for needing to use ExtendedNumberParser (unconditionally) instead of SqNumberParser ... the other two cases, I think I understand ...

As always a pull request will be integrated into the release much quicker than waiting for me to get around to dealing with the issue:)

Dale

[1] https://github.com/GsDevKit/GsDevKit/issues

Do you know if issues from glassdb were migrated?

I cannot find for example, this issue:   https://github.com/glassdb/glass/issues/20

Thanks,

 

On 09/08/2015 07:27 AM, Mariano Martinez Peck via Glass wrote:

On Tue, Sep 8, 2015 at 6:50 AM, Iwan Vosloo via Glass <[hidden email]> wrote:

On 07/09/2015 16:56, Dale Henrichs via Glass wrote:

It turns out that in the GemStone/GsDevKit version of NumberParser (actually SqNumberParser), isNumber: is not implemented ... was there a major rewrite of NumberParser in Pharo4.0 (it's not in Pharo3.0 either) or is it a simple extension method?

I might be able to squeeze out some time and take a bigger look at this one:)

From what I can see, Magritte-Model adds isNumber in the Pharo and GemStone case. The real problem is perhaps how isNumber: is implemented OR perhaps the implementation of parse:onError: which it sends.

Hi guys, 

This is not 100% same topic, but wanted to say it in case we fix both. Unfortunately, I cannot remember what was broken, and I did not even wrote a comment....but as part of my sad "overrides" in GemStone, I have an override of Integer >> #readFrom: 

 

Integer >> readFrom: aStringOrStream 

"Answer a new Integer as described on the stream, aStream.

Embedded radix specifiers not allowed - use Number readFrom: for that."

^self readFrom: aStringOrStream base: 10

And I had to override it like this:

Integer >> faReadFrom: aStream ^ (SqNumberParser on: aStream) nextIntegerBase: 10

And I have to also change:

SqNumberParser class >> on: aStringOrStream

^ExtendedNumberParser new on: aStringOrStream

SqNumberParser >> exponentLetters

"answer the list of possible exponents for Numbers.

Note: this parser will not honour precision attached to the exponent.

different exponent do not lead to different precisions.

only IEEE 754 floating point numbers will be created"

^'edqEDQ'

Regards
- Iwan

--
Reahl, the Python only web framework: http://www.reahl.org

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

--

Mariano
http://marianopeck.wordpress.com

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

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

--

Mariano
http://marianopeck.wordpress.com

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

As always a pull request will be integrated into the release much quicker than waiting for me to get around to dealing with the issue:)

A crazy thought I know, but a pull request with a failing test would also cause corresponding bugfixes to be integrated quicker than waiting for me to "get around to it" ...

Dale

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

Hi Dale,

On 08/09/2015 21:48, Dale Henrichs via Glass wrote:
> Okay, then I need to dive into the Magritte implementation directly
> and try to figure things out from there ... I naively thought that the
> problem was the direct use of NumberParser in the code, instead of
> SqNumberParser - it would be relatively easy to change NumberParser to
> redirect instance creation to SqNumberParser, but It sounds like that
> isn't the source ofthe problem ...

AFAIK the implementation of NumberParser>>isNumber: should not call
parse:onError: on a hardcoded NumberParser, but rather on self. (This
regardless of platform.)

But then MAStringReader>>visitNumberDescription: should also rather call
isNumber: on SqNumberParser as you said above. (In the GemStone case.)

>
> BTW are Magritte tests failing on GemStone or is this an untested
> chunk of code?
There are a couple of failures if I run all tests on
MANumberDescriptionTest, including testFromString which fails exactly
because of this issue.


I have not tried to figure out how Mariano's posting is related to this.

Regards
- Iwan

--
Reahl, the Python only web framework: http://www.reahl.org

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

Iwan,

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...

With the flurry of emails in the last day, my todo list is building faster than I can knock items off the list so I don't know when I can find the time to make the changes myself ...

If you could a) verify that this approach would work and b) submit a pull request against https://github.com/glassdb/glass with your changes then we could get this issue resolved for everyone ...
 
Dale

Hi Dale,

On 08/09/2015 21:48, Dale Henrichs via Glass wrote:

Okay, then I need to dive into the Magritte implementation directly and try to figure things out from there ... I naively thought that the problem was the direct use of NumberParser in the code, instead of SqNumberParser - it would be relatively easy to change NumberParser to redirect instance creation to SqNumberParser, but It sounds like that isn't the source ofthe problem ...

AFAIK the implementation of NumberParser>>isNumber: should not call parse:onError: on a hardcoded NumberParser, but rather on self. (This regardless of platform.)

But then MAStringReader>>visitNumberDescription: should also rather call isNumber: on SqNumberParser as you said above. (In the GemStone case.)

BTW are Magritte tests failing on GemStone or is this an untested chunk of code?

There are a couple of failures if I run all tests on MANumberDescriptionTest, including testFromString which fails exactly because of this issue.


I have not tried to figure out how Mariano's posting is related to this.

Regards
- Iwan

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

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...

Hi Dale,

I see your repo at: xxx contains three different copies of the Squeak
package (which is where the relevant code and tests reside). They are:
Squeak, Squeak.v32, Squeak.v33, Squeak.v3.

In which one of these should I make changes?


Regards
- Iwan

--
Reahl, the Python only web framework: http://www.reahl.org

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

On 14/09/2015 11:15, Iwan Vosloo wrote:
> Hi Dale,
>
> I see your repo at: xxx contains three different copies of the Squeak
> package (which is where the relevant code and tests reside). They are:
> Squeak, Squeak.v32, Squeak.v33, Squeak.v3.
>
> In which one of these should I make changes?
Whoops, I meant repo at https://github.com/glassdb/glass

Regards
- Iwan

--
Reahl, the Python only web framework: http://www.reahl.org

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

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.

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?

Anyways - if I do understand you correctly - this does solve the problem
without a change to NumberParser class>>isNumber: (which is Magritte
code itself)

Regards
- Iwan


--
Reahl, the Python only web framework: http://www.reahl.org

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

Iwan,

Technically the change needs to be made in all three, but if you have a
pull request with the changes in Squeak.v3, I'll take care of the rest:)

Dale

On 09/14/2015 02:18 AM, Iwan Vosloo via Glass wrote:
_______________________________________________
Glass mailing list
[hidden email]
http://lists.gemtalksystems.com/mailman/listinfo/glass

On 09/14/2015 02:54 AM, Iwan Vosloo via Glass wrote: 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