Smalltalk › Squeak › Squeak - Dev

The Trunk: Tests-cmm.290.mcz

‹ Previous Topic Next Topic ›
 
classic Classic list List threaded Threaded
♦
♦
 7 messages Options
commits-2
Reply | Threaded
Open this post in threaded view
♦
♦
|

The Trunk: Tests-cmm.290.mcz

commits-2
Chris Muller uploaded a new version of Tests to project The Trunk:
http://source.squeak.org/trunk/Tests-cmm.290.mcz

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

Name: Tests-cmm.290
Author: cmm
Time: 17 February 2014, 4:35:24.565 pm
UUID: f3fccfae-6baf-4093-ba62-e15ef110a687
Ancestors: Tests-cwp.289

Minor factoring.

=============== Diff against Tests-cwp.289 ===============

Item was changed:
  ----- Method: BlockLocalTemporariesRemovalTest>>assert:isChangedDuringParsingTo:withRemovalOfTemporariesNamed: (in category 'test helper') -----
+ assert: someCode isChangedDuringParsingTo: someOtherCode withRemovalOfTemporariesNamed: someTempNames
- assert: someCode isChangedDuringParsingTo: someOtherCode withRemovalOfTemporariesNamed: someTempNames
-
  | failBlock |
  self sourceCode: someCode.
+ failBlock := [ self fail ].
+ [ self class
+ compile: self sourceCode
+ notifying: self
+ trailer: self class defaultMethodTrailer
+ ifFail: failBlock ]
- failBlock := [self fail].
- [self class
- compile: self sourceCode
- notifying: self
- trailer: self class defaultMethodTrailer
- ifFail: failBlock]
  on: UnusedVariable
+ do:
+ [ : aNotification | aNotification openMenuIn:
+ [ : options : emptyCollection : someText | aNotification resume:
+ (someTempNames anySatisfy:
+ [ : tempName | someText beginsWith: tempName ]) ] ].
- do: [:aNotification | aNotification
- openMenuIn: [:options :emptyCollection :someText |
- (someTempNames anySatisfy: [:tempName | someText startsWith: tempName])
- ifTrue: [aNotification resume: true]
- ifFalse: [aNotification resume: false]]].
  self assert: self sourceCode = someOtherCode!


Frank Shearar-3
Reply | Threaded
Open this post in threaded view
♦
♦
|

Re: The Trunk: Tests-cmm.290.mcz

Frank Shearar-3
On 17 February 2014 22:35,  <[hidden email]> wrote:

> Chris Muller uploaded a new version of Tests to project The Trunk:
> http://source.squeak.org/trunk/Tests-cmm.290.mcz
>
> ==================== Summary ====================
>
> Name: Tests-cmm.290
> Author: cmm
> Time: 17 February 2014, 4:35:24.565 pm
> UUID: f3fccfae-6baf-4093-ba62-e15ef110a687
> Ancestors: Tests-cwp.289
>
> Minor factoring.
>
> =============== Diff against Tests-cwp.289 ===============
>
> Item was changed:
>   ----- Method: BlockLocalTemporariesRemovalTest>>assert:isChangedDuringParsingTo:withRemovalOfTemporariesNamed: (in category 'test helper') -----
> + assert: someCode isChangedDuringParsingTo: someOtherCode withRemovalOfTemporariesNamed: someTempNames
> - assert: someCode isChangedDuringParsingTo: someOtherCode withRemovalOfTemporariesNamed: someTempNames
> -
>         | failBlock |
>         self sourceCode: someCode.
> +       failBlock := [ self fail ].
> +       [ self class
> +               compile: self sourceCode
> +               notifying: self
> +               trailer: self class defaultMethodTrailer
> +               ifFail: failBlock ]
> -       failBlock := [self fail].
> -       [self class
> -                       compile: self sourceCode
> -                       notifying: self
> -                       trailer: self class defaultMethodTrailer
> -                       ifFail: failBlock]
>                 on: UnusedVariable
> +               do:
> +                       [ : aNotification | aNotification openMenuIn:
> +                               [ : options : emptyCollection : someText | aNotification resume:
> +                                       (someTempNames anySatisfy:
> +                                               [ : tempName | someText beginsWith: tempName ]) ] ].
> -               do: [:aNotification | aNotification
> -                               openMenuIn: [:options :emptyCollection :someText |
> -                                       (someTempNames anySatisfy: [:tempName | someText startsWith: tempName])
> -                                               ifTrue: [aNotification resume: true]
> -                                               ifFalse: [aNotification resume: false]]].
>         self assert: self sourceCode = someOtherCode!

Seriously, we need a better differ. If we pretty-printed the before &
after I'd at least stand a chance of not going blind trying to find
the change here. How difficult a change is it to SS? Surely a
one-liner, to someone who knows the codebase (i.e., not me).

frank

Tobias Pape
Reply | Threaded
Open this post in threaded view
♦
♦
|

Re: The Trunk: Tests-cmm.290.mcz

Tobias Pape
On 18.02.2014, at 23:21, Frank Shearar <[hidden email]> wrote:

> On 17 February 2014 22:35,  <[hidden email]> wrote:
>> Chris Muller uploaded a new version of Tests to project The Trunk:
>> http://source.squeak.org/trunk/Tests-cmm.290.mcz
>>
>> ==================== Summary ====================
>>
>> Name: Tests-cmm.290
>> Author: cmm
>> Time: 17 February 2014, 4:35:24.565 pm
>> UUID: f3fccfae-6baf-4093-ba62-e15ef110a687
>> Ancestors: Tests-cwp.289
>>
>> Minor factoring.
>>
>> =============== Diff against Tests-cwp.289 ===============
>>
>> Item was changed:
>>  ----- Method: BlockLocalTemporariesRemovalTest>>assert:isChangedDuringParsingTo:withRemovalOfTemporariesNamed: (in category 'test helper') -----
>> + assert: someCode isChangedDuringParsingTo: someOtherCode withRemovalOfTemporariesNamed: someTempNames
>> - assert: someCode isChangedDuringParsingTo: someOtherCode withRemovalOfTemporariesNamed: someTempNames
>> -
>>        | failBlock |
>>        self sourceCode: someCode.
>> +       failBlock := [ self fail ].
>> +       [ self class
>> +               compile: self sourceCode
>> +               notifying: self
>> +               trailer: self class defaultMethodTrailer
>> +               ifFail: failBlock ]
>> -       failBlock := [self fail].
>> -       [self class
>> -                       compile: self sourceCode
>> -                       notifying: self
>> -                       trailer: self class defaultMethodTrailer
>> -                       ifFail: failBlock]
>>                on: UnusedVariable
>> +               do:
>> +                       [ : aNotification | aNotification openMenuIn:
>> +                               [ : options : emptyCollection : someText | aNotification resume:
>> +                                       (someTempNames anySatisfy:
>> +                                               [ : tempName | someText beginsWith: tempName ]) ] ].
>> -               do: [:aNotification | aNotification
>> -                               openMenuIn: [:options :emptyCollection :someText |
>> -                                       (someTempNames anySatisfy: [:tempName | someText startsWith: tempName])
>> -                                               ifTrue: [aNotification resume: true]
>> -                                               ifFalse: [aNotification resume: false]]].
>>        self assert: self sourceCode = someOtherCode!
>
> Seriously, we need a better differ. If we pretty-printed the before &
> after I'd at least stand a chance of not going blind trying to find
> the change here. How difficult a change is it to SS? Surely a
> one-liner, to someone who knows the codebase (i.e., not me).
"MCDiffyTextBuilder>>writePatchFrom:to:"
 writePatchFrom: src to: dst
  "src and dst are allowed to bi nil to represent a non-existent source or destination state"
 
  | source target excess |
  source := src ifNotNil: [self visitInFork: src] ifNil: [''].
  target := dst ifNotNil: [self visitInFork: dst] ifNil: [''].
  excess := source size + target size > 20000.
  excess ifTrue: [
  "show at least header line"
  source := source copyUpTo: Character cr.
  target := target copyUpTo: Character cr ].
  "there might have been no CR, check again"
  source size + target size > 20000
  ifFalse: [
- stream nextPutAll: (TextDiffBuilder from: source to: target)
+ stream nextPutAll: (PrettyTextDiffBuilder from: source to: target)
  buildTextPatch ].
  "alert audience"
  excess ifTrue: [
  stream nextPutAll: '(excessive size, no diff calculated)'; cr ]

But I _really_ think we should _not_ do that :)

Best
        -Tobias




signature.asc (1K) Download Attachment
Chris Muller-3
Reply | Threaded
Open this post in threaded view
♦
♦
|

Re: The Trunk: Tests-cmm.290.mcz

Chris Muller-3
In reply to this post by Frank Shearar-3
The upper set of +'s is a non-change -- just a side-effect of my
having used code-format because I was too lazy to manually format this
time.  Normally I only reformat when I drastically change a method,
sometimes I get lazy though.

Bottom change simply factor "aNotification resume:" out of the conditional.

PS -- Code for our SqueakSource is actually in the same instance in
case you wish to browse it:

MCHttpRepository
    location: 'http://source.squeak.org/ss'
    user: ''
    password: ''



On Tue, Feb 18, 2014 at 4:21 PM, Frank Shearar <[hidden email]> wrote:

> On 17 February 2014 22:35,  <[hidden email]> wrote:
>> Chris Muller uploaded a new version of Tests to project The Trunk:
>> http://source.squeak.org/trunk/Tests-cmm.290.mcz
>>
>> ==================== Summary ====================
>>
>> Name: Tests-cmm.290
>> Author: cmm
>> Time: 17 February 2014, 4:35:24.565 pm
>> UUID: f3fccfae-6baf-4093-ba62-e15ef110a687
>> Ancestors: Tests-cwp.289
>>
>> Minor factoring.
>>
>> =============== Diff against Tests-cwp.289 ===============
>>
>> Item was changed:
>>   ----- Method: BlockLocalTemporariesRemovalTest>>assert:isChangedDuringParsingTo:withRemovalOfTemporariesNamed: (in category 'test helper') -----
>> + assert: someCode isChangedDuringParsingTo: someOtherCode withRemovalOfTemporariesNamed: someTempNames
>> - assert: someCode isChangedDuringParsingTo: someOtherCode withRemovalOfTemporariesNamed: someTempNames
>> -
>>         | failBlock |
>>         self sourceCode: someCode.
>> +       failBlock := [ self fail ].
>> +       [ self class
>> +               compile: self sourceCode
>> +               notifying: self
>> +               trailer: self class defaultMethodTrailer
>> +               ifFail: failBlock ]
>> -       failBlock := [self fail].
>> -       [self class
>> -                       compile: self sourceCode
>> -                       notifying: self
>> -                       trailer: self class defaultMethodTrailer
>> -                       ifFail: failBlock]
>>                 on: UnusedVariable
>> +               do:
>> +                       [ : aNotification | aNotification openMenuIn:
>> +                               [ : options : emptyCollection : someText | aNotification resume:
>> +                                       (someTempNames anySatisfy:
>> +                                               [ : tempName | someText beginsWith: tempName ]) ] ].
>> -               do: [:aNotification | aNotification
>> -                               openMenuIn: [:options :emptyCollection :someText |
>> -                                       (someTempNames anySatisfy: [:tempName | someText startsWith: tempName])
>> -                                               ifTrue: [aNotification resume: true]
>> -                                               ifFalse: [aNotification resume: false]]].
>>         self assert: self sourceCode = someOtherCode!
>
> Seriously, we need a better differ. If we pretty-printed the before &
> after I'd at least stand a chance of not going blind trying to find
> the change here. How difficult a change is it to SS? Surely a
> one-liner, to someone who knows the codebase (i.e., not me).
>
> frank
>

Nicolas Cellier
Reply | Threaded
Open this post in threaded view
♦
♦
|

Re: The Trunk: Tests-cmm.290.mcz

Nicolas Cellier
In reply to this post by Frank Shearar-3



2014-02-18 23:21 GMT+01:00 Frank Shearar <[hidden email]>:
On 17 February 2014 22:35,  <[hidden email]> wrote:
> Chris Muller uploaded a new version of Tests to project The Trunk:
> http://source.squeak.org/trunk/Tests-cmm.290.mcz
>
> ==================== Summary ====================
>
> Name: Tests-cmm.290
> Author: cmm
> Time: 17 February 2014, 4:35:24.565 pm
> UUID: f3fccfae-6baf-4093-ba62-e15ef110a687
> Ancestors: Tests-cwp.289
>
> Minor factoring.
>
> =============== Diff against Tests-cwp.289 ===============
>
> Item was changed:
>   ----- Method: BlockLocalTemporariesRemovalTest>>assert:isChangedDuringParsingTo:withRemovalOfTemporariesNamed: (in category 'test helper') -----
> + assert: someCode isChangedDuringParsingTo: someOtherCode withRemovalOfTemporariesNamed: someTempNames
> - assert: someCode isChangedDuringParsingTo: someOtherCode withRemovalOfTemporariesNamed: someTempNames
> -
>         | failBlock |
>         self sourceCode: someCode.
> +       failBlock := [ self fail ].
> +       [ self class
> +               compile: self sourceCode
> +               notifying: self
> +               trailer: self class defaultMethodTrailer
> +               ifFail: failBlock ]
> -       failBlock := [self fail].
> -       [self class
> -                       compile: self sourceCode
> -                       notifying: self
> -                       trailer: self class defaultMethodTrailer
> -                       ifFail: failBlock]
>                 on: UnusedVariable
> +               do:
> +                       [ : aNotification | aNotification openMenuIn:
> +                               [ : options : emptyCollection : someText | aNotification resume:
> +                                       (someTempNames anySatisfy:
> +                                               [ : tempName | someText beginsWith: tempName ]) ] ].
> -               do: [:aNotification | aNotification
> -                               openMenuIn: [:options :emptyCollection :someText |
> -                                       (someTempNames anySatisfy: [:tempName | someText startsWith: tempName])
> -                                               ifTrue: [aNotification resume: true]
> -                                               ifFalse: [aNotification resume: false]]].
>         self assert: self sourceCode = someOtherCode!

Seriously, we need a better differ. If we pretty-printed the before &
after I'd at least stand a chance of not going blind trying to find
the change here. How difficult a change is it to SS? Surely a
one-liner, to someone who knows the codebase (i.e., not me).

frank


Frank, in this case you have to trust the comment: minot refactoring
(ifTrue: [true] ifFalse: [false]), there must be something simpler indeed...
Frank Shearar-3
Reply | Threaded
Open this post in threaded view
♦
♦
|

Re: The Trunk: Tests-cmm.290.mcz

Frank Shearar-3
In reply to this post by commits-2
On 17 February 2014 22:35,  <[hidden email]> wrote:

> Chris Muller uploaded a new version of Tests to project The Trunk:
> http://source.squeak.org/trunk/Tests-cmm.290.mcz
>
> ==================== Summary ====================
>
> Name: Tests-cmm.290
> Author: cmm
> Time: 17 February 2014, 4:35:24.565 pm
> UUID: f3fccfae-6baf-4093-ba62-e15ef110a687
> Ancestors: Tests-cwp.289
>
> Minor factoring.
>
> =============== Diff against Tests-cwp.289 ===============
>
> Item was changed:
>   ----- Method: BlockLocalTemporariesRemovalTest>>assert:isChangedDuringParsingTo:withRemovalOfTemporariesNamed: (in category 'test helper') -----
> + assert: someCode isChangedDuringParsingTo: someOtherCode withRemovalOfTemporariesNamed: someTempNames
> - assert: someCode isChangedDuringParsingTo: someOtherCode withRemovalOfTemporariesNamed: someTempNames
> -
>         | failBlock |
>         self sourceCode: someCode.
> +       failBlock := [ self fail ].
> +       [ self class
> +               compile: self sourceCode
> +               notifying: self
> +               trailer: self class defaultMethodTrailer
> +               ifFail: failBlock ]
> -       failBlock := [self fail].
> -       [self class
> -                       compile: self sourceCode
> -                       notifying: self
> -                       trailer: self class defaultMethodTrailer
> -                       ifFail: failBlock]
>                 on: UnusedVariable
> +               do:
> +                       [ : aNotification | aNotification openMenuIn:
> +                               [ : options : emptyCollection : someText | aNotification resume:
> +                                       (someTempNames anySatisfy:
> +                                               [ : tempName | someText beginsWith: tempName ]) ] ].

I normally hold my tongue (ok, it's me, I _bite_ my tongue), but in
this case I find it really hard to read this block when there's a
message send in the same line as the temp declaration.

I'd expect to see this:

    [:options :emptyCollection :someText |
        aNotification resume:

Other than that, the change looks great.

(Chris replaced the boolExpr ifTrue: [aNotification resume: true]
ifFalse: [aNotification resume: false] with aNotification resume:
boolExpr)

frank

> -               do: [:aNotification | aNotification
> -                               openMenuIn: [:options :emptyCollection :someText |
> -                                       (someTempNames anySatisfy: [:tempName | someText startsWith: tempName])
> -                                               ifTrue: [aNotification resume: true]
> -                                               ifFalse: [aNotification resume: false]]].
>         self assert: self sourceCode = someOtherCode!
>
>

Chris Muller-3
Reply | Threaded
Open this post in threaded view
♦
♦
|

Re: The Trunk: Tests-cmm.290.mcz

Chris Muller-3
Totally agree.  It's a bug in the formatter I wasn't able to fix when
I tried once a while back.

On Tue, Feb 18, 2014 at 4:43 PM, Frank Shearar <[hidden email]> wrote:

> On 17 February 2014 22:35,  <[hidden email]> wrote:
>> Chris Muller uploaded a new version of Tests to project The Trunk:
>> http://source.squeak.org/trunk/Tests-cmm.290.mcz
>>
>> ==================== Summary ====================
>>
>> Name: Tests-cmm.290
>> Author: cmm
>> Time: 17 February 2014, 4:35:24.565 pm
>> UUID: f3fccfae-6baf-4093-ba62-e15ef110a687
>> Ancestors: Tests-cwp.289
>>
>> Minor factoring.
>>
>> =============== Diff against Tests-cwp.289 ===============
>>
>> Item was changed:
>>   ----- Method: BlockLocalTemporariesRemovalTest>>assert:isChangedDuringParsingTo:withRemovalOfTemporariesNamed: (in category 'test helper') -----
>> + assert: someCode isChangedDuringParsingTo: someOtherCode withRemovalOfTemporariesNamed: someTempNames
>> - assert: someCode isChangedDuringParsingTo: someOtherCode withRemovalOfTemporariesNamed: someTempNames
>> -
>>         | failBlock |
>>         self sourceCode: someCode.
>> +       failBlock := [ self fail ].
>> +       [ self class
>> +               compile: self sourceCode
>> +               notifying: self
>> +               trailer: self class defaultMethodTrailer
>> +               ifFail: failBlock ]
>> -       failBlock := [self fail].
>> -       [self class
>> -                       compile: self sourceCode
>> -                       notifying: self
>> -                       trailer: self class defaultMethodTrailer
>> -                       ifFail: failBlock]
>>                 on: UnusedVariable
>> +               do:
>> +                       [ : aNotification | aNotification openMenuIn:
>> +                               [ : options : emptyCollection : someText | aNotification resume:
>> +                                       (someTempNames anySatisfy:
>> +                                               [ : tempName | someText beginsWith: tempName ]) ] ].
>
> I normally hold my tongue (ok, it's me, I _bite_ my tongue), but in
> this case I find it really hard to read this block when there's a
> message send in the same line as the temp declaration.
>
> I'd expect to see this:
>
>     [:options :emptyCollection :someText |
>         aNotification resume:
>
> Other than that, the change looks great.
>
> (Chris replaced the boolExpr ifTrue: [aNotification resume: true]
> ifFalse: [aNotification resume: false] with aNotification resume:
> boolExpr)
>
> frank
>
>> -               do: [:aNotification | aNotification
>> -                               openMenuIn: [:options :emptyCollection :someText |
>> -                                       (someTempNames anySatisfy: [:tempName | someText startsWith: tempName])
>> -                                               ifTrue: [aNotification resume: true]
>> -                                               ifFalse: [aNotification resume: false]]].
>>         self assert: self sourceCode = someOtherCode!
>>
>>
>

Free forum by Nabble Edit this page