The Inbox: ShoutCore-nice.63.mcz

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

The Inbox: ShoutCore-nice.63.mcz

commits-2
Nicolas Cellier uploaded a new version of ShoutCore to project The Inbox:
http://source.squeak.org/inbox/ShoutCore-nice.63.mcz

==================== Summary ====================

Name: ShoutCore-nice.63
Author: nice
Time: 7 April 2019, 3:24:31.721529 pm
UUID: 7440b898-b497-493b-adfa-67aef04e0980
Ancestors: ShoutCore-eem.62

Attempt to fix highlighting of 1to:-1

I don't see the point of testing the presence of a binary selector character after a colon $:, except maybe the case of assignment :=, so only test that case.

=============== Diff against ShoutCore-eem.62 ===============

Item was changed:
  ----- Method: SHParserST80>>scanIdentifier (in category 'scan') -----
  scanIdentifier
 
  | c start |
  start := sourcePosition.
  [
  (c := self nextChar) isAlphaNumeric
  or: [ allowUnderscoreSelectors and: [ c == $_ ] ] ] whileTrue.
+ (c == $: and: [ self peekChar ~= $= ])
- (c == $: and: [ (self isSelectorCharacter: self peekChar) not ])
  ifTrue: [ self nextChar ].
  currentToken := source copyFrom: start to: sourcePosition - 1.
  currentTokenSourcePosition := start!


Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: ShoutCore-nice.63.mcz

Nicolas Cellier
There are not many ShoutTests that could ensure that I'm no the right track...
And Shout code is hard to read for me (see below).
It's also strange to have to change such a central method after all these years of service.
Many reasons for a stage in inbox, if ever a few pairs of eyes are browsing there...

<rant>
I find this Shout Parser selector particularly misleading...
It's hard to guess that #isSelectorCharacter:  is going to check exclusively for binary selectors before I browse the implementation details.
Should it be named #isBinarySelectorCharacter:, I could eventually omit such browsing.

In a lesser way, I also do not like self isName, self isBinary, self isKeyword,...
The intention is to check for the currentToken, not self.
They have the advantage of being short, but reading the code gives a strange taste.
If I wanted to avoid the heavy isCurrentTokenAName, isCurrenTokenABinarySelector, isCurrentTokenAKeyword, ... maybe I would just use has instead of is?

I've also ran the Shout Parser through a Debugger, and I encountered a lot of redundancy during the tokens scan (those isName isKeyword... are ran several consecutive times).
</rant>

Le dim. 7 avr. 2019 à 15:24, <[hidden email]> a écrit :
Nicolas Cellier uploaded a new version of ShoutCore to project The Inbox:
http://source.squeak.org/inbox/ShoutCore-nice.63.mcz

==================== Summary ====================

Name: ShoutCore-nice.63
Author: nice
Time: 7 April 2019, 3:24:31.721529 pm
UUID: 7440b898-b497-493b-adfa-67aef04e0980
Ancestors: ShoutCore-eem.62

Attempt to fix highlighting of 1to:-1

I don't see the point of testing the presence of a binary selector character after a colon $:, except maybe the case of assignment :=, so only test that case.

=============== Diff against ShoutCore-eem.62 ===============

Item was changed:
  ----- Method: SHParserST80>>scanIdentifier (in category 'scan') -----
  scanIdentifier

        | c start |
        start := sourcePosition.
        [
                (c := self nextChar) isAlphaNumeric
                        or: [ allowUnderscoreSelectors and: [ c == $_ ] ] ] whileTrue.
+       (c == $: and: [ self peekChar ~= $= ])
-       (c == $: and: [ (self isSelectorCharacter: self peekChar) not ])
                ifTrue: [ self nextChar ].
        currentToken := source copyFrom: start to: sourcePosition - 1.
        currentTokenSourcePosition := start!




Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: ShoutCore-nice.63.mcz

Levente Uzonyi
On Sun, 7 Apr 2019, Nicolas Cellier wrote:

> There are not many ShoutTests that could ensure that I'm no the right track...
> And Shout code is hard to read for me (see below).
> It's also strange to have to change such a central method after all these years of service.
> Many reasons for a stage in inbox, if ever a few pairs of eyes are browsing there...

The change looks correct and works. I suggest it be pushed to the Trunk.

>
> <rant>
> I find this Shout Parser selector particularly misleading...
> It's hard to guess that #isSelectorCharacter:  is going to check exclusively for binary selectors before I browse the implementation details.
> Should it be named #isBinarySelectorCharacter:, I could eventually omit such browsing.
>
> In a lesser way, I also do not like self isName, self isBinary, self isKeyword,...
> The intention is to check for the currentToken, not self.
> They have the advantage of being short, but reading the code gives a strange taste.
> If I wanted to avoid the heavy isCurrentTokenAName, isCurrenTokenABinarySelector, isCurrentTokenAKeyword, ... maybe I would just use has instead of is?
>
> I've also ran the Shout Parser through a Debugger, and I encountered a lot of redundancy during the tokens scan (those isName isKeyword... are ran several consecutive times).
I started rewriting these methods. I'll push the changes to the inbox
as soon as I'm happy with them.

Levente

> </rant>
>
> Le dim. 7 avr. 2019 à 15:24, <[hidden email]> a écrit :
>       Nicolas Cellier uploaded a new version of ShoutCore to project The Inbox:
>       http://source.squeak.org/inbox/ShoutCore-nice.63.mcz
>
>       ==================== Summary ====================
>
>       Name: ShoutCore-nice.63
>       Author: nice
>       Time: 7 April 2019, 3:24:31.721529 pm
>       UUID: 7440b898-b497-493b-adfa-67aef04e0980
>       Ancestors: ShoutCore-eem.62
>
>       Attempt to fix highlighting of 1to:-1
>
>       I don't see the point of testing the presence of a binary selector character after a colon $:, except maybe the case of assignment :=, so only test that case.
>
>       =============== Diff against ShoutCore-eem.62 ===============
>
>       Item was changed:
>         ----- Method: SHParserST80>>scanIdentifier (in category 'scan') -----
>         scanIdentifier
>
>               | c start |
>               start := sourcePosition.
>               [
>                       (c := self nextChar) isAlphaNumeric
>                               or: [ allowUnderscoreSelectors and: [ c == $_ ] ] ] whileTrue.
>       +       (c == $: and: [ self peekChar ~= $= ])
>       -       (c == $: and: [ (self isSelectorCharacter: self peekChar) not ])
>                       ifTrue: [ self nextChar ].
>               currentToken := source copyFrom: start to: sourcePosition - 1.
>               currentTokenSourcePosition := start!
>
>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: ShoutCore-nice.63.mcz

Levente Uzonyi
On Thu, 11 Apr 2019, Levente Uzonyi wrote:

> On Sun, 7 Apr 2019, Nicolas Cellier wrote:
>
>>
>> <rant>
>> I find this Shout Parser selector particularly misleading...
>> It's hard to guess that #isSelectorCharacter:  is going to check
>> exclusively for binary selectors before I browse the implementation
>> details.
>> Should it be named #isBinarySelectorCharacter:, I could eventually omit
>> such browsing.
>>
>> In a lesser way, I also do not like self isName, self isBinary, self
>> isKeyword,...
>> The intention is to check for the currentToken, not self.
>> They have the advantage of being short, but reading the code gives a
>> strange taste.
>> If I wanted to avoid the heavy isCurrentTokenAName,
>> isCurrenTokenABinarySelector, isCurrentTokenAKeyword, ... maybe I would
>> just use has instead of is?
>>
>> I've also ran the Shout Parser through a Debugger, and I encountered a lot
>> of redundancy during the tokens scan (those isName isKeyword... are ran
>> several consecutive times).
>
> I started rewriting these methods. I'll push the changes to the inbox as soon
> as I'm happy with them.
It took a bit longer than I expected and it grew a lot larger as well but
it's finally in the inbox as ShoutCore-ul.66. You'll have to load
Collections-ul.844 before that. Some tests needed adjustments which are
included in ShoutTests-ul.29.

Levente