Magritte-Model changes .455, .456

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Magritte-Model changes .455, .456

Stephan Eggermont
I'm not sure the replacement of

visitNumberDescription: aDescription
     | contents |
     contents := self contents.
     contents isEmpty
         ifTrue: [ MAReadError signal ].
     (contents occurrencesOf: $-) > 1
         ifTrue: [ MAReadError signal ].
     (contents indexOf: $-) > 1
         ifTrue: [ MAReadError signal ].
     (contents occurrencesOf: $.) > 1
         ifTrue: [ MAReadError signal ].
     (contents allSatisfy: [ :each | '+-0123456789.eE' includes: each ])
         ifFalse: [ MAReadError signal ].
     super visitNumberDescription: aDescription

by
visitNumberDescription: aDescription
     | isContentsValid |
     isContentsValid := NumberParser isNumber: self contents.
     isContentsValid ifFalse: [ MAReadError signal ].
     super visitNumberDescription: aDescription

works cross-platform.

There are two 455s, and 456 misses changes from both.
Merged in 457

Stephan


_______________________________________________
Magritte, Pier and Related Tools ...
https://www.iam.unibe.ch/mailman/listinfo/smallwiki
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Magritte-Model changes .455, .456

Stephan Eggermont
Hmm, I missed some more changes.

I'm not sure I'd want the MABlockDescription in Magritte-Model.
Perhaps a separate package Magritte-UnsafeExtensions?

"protocol: visiting-description"
MAStringReader>>visitBlockDescription: aDescription
     | block |
     block := Compiler evaluate: self contents.
     self object: block

458 provides a better merge, without th eblock description.
Still not compatible with Pharo20 build (NumberParser)

Stephan
_______________________________________________
Magritte, Pier and Related Tools ...
https://www.iam.unibe.ch/mailman/listinfo/smallwiki
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Magritte-Model changes .455, .456

Sean P. DeNigris
Administrator
Stephan Eggermont wrote
I'm not sure I'd want the MABlockDescription in Magritte-Model.
Perhaps a separate package Magritte-UnsafeExtensions?
Thanks for the review! Perhaps Magritte-ModelUnsafe? But it seems to me that if you're allowing users to type code blocks in a form, you have to know that there are security concerns e.g. [ FileLocator home... "delete all files" ]
Cheers,
Sean
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Magritte-Model changes .455, .456

Sean P. DeNigris
Administrator
In reply to this post by Stephan Eggermont
Stephan Eggermont wrote
I'm not sure the replacement... works cross-platform
Thanks for this review too :) I'm never quite sure which platforms we expect to be supported, so I refactor and brace for complaints! When I was making the change, I also considered a Magritte extension on Number or String (which could more easily be made cross-platform), but couldn't think of a good message name - #representsNumber?
Cheers,
Sean
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Magritte-Model changes .455, .456

Stephan Eggermont
In reply to this post by Sean P. DeNigris
ModelUnsafe sounds good to me. That makes the decision to
live dangerously explicit...

Stephan

On 17-04-15 13:49, Sean P. DeNigris wrote:

> Stephan Eggermont wrote
>> I'm not sure I'd want the MABlockDescription in Magritte-Model.
>> Perhaps a separate package Magritte-UnsafeExtensions?
> Thanks for the review! Perhaps Magritte-ModelUnsafe? But it seems to me that
> if you're allowing users to type code blocks in a form, you have to know
> that there are security concerns e.g. [ FileLocator home... "delete all
> files" ]
>
>
>
> -----
> Cheers,
> Sean
> --
> View this message in context: http://forum.world.st/Magritte-Model-changes-455-456-tp4819848p4820193.html
> Sent from the Magritte, Pier and Related Tools mailing list archive at Nabble.com.
> _______________________________________________
> Magritte, Pier and Related Tools ...
> https://www.iam.unibe.ch/mailman/listinfo/smallwiki

_______________________________________________
Magritte, Pier and Related Tools ...
https://www.iam.unibe.ch/mailman/listinfo/smallwiki
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Magritte-Model changes .455, .456

Sean P. DeNigris
Administrator
In reply to this post by Stephan Eggermont
Stephan Eggermont wrote
I'm not sure the replacement of

visitNumberDescription: aDescription
...
     (contents allSatisfy: [ :each | '+-0123456789.eE' includes: each ])
Another reason to rely on the platform's number parsing and not reinvent our own logic. Pharo has ScaledDecimals, which use $s in the literal form and printString, which is not reflected in our tests. To further complicate, the error generated was silently swallowed as reported in another thread a moment ago.
Cheers,
Sean
Loading...