First commit to Pharoinbox. Need advice.

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

First commit to Pharoinbox. Need advice.

HwaJong Oh
My last package is Balloon-hjo.72.mcz.
It is small testing & refactoring on Ballloon supporting collections.

I wonder this type of work is acceptable?

Best Regards
Reply | Threaded
Open this post in threaded view
|

Re: First commit to Pharoinbox. Need advice.

Igor Stasenko
On 26 January 2011 08:13, HwaJong Oh <[hidden email]> wrote:
>
> My last package is Balloon-hjo.72.mcz.
> It is small testing & refactoring on Ballloon supporting collections.
>
> I wonder this type of work is acceptable?

Are you joking? Any contribution is acceptable. Of course if its pass
the quality test :)
And anything which improves functionality & cleans the code is acceptable.

Can i ask you to make life easier for review?
Please create entry in issue tracker , put a link to file there and a
little description of your changes.
This is to make life easier for people who will review your code and
integrate it. :)
Also, this will make integration process visible to all people.

>
> Best Regards
> --
> View this message in context: http://forum.world.st/First-commit-to-Pharoinbox-Need-advice-tp3237535p3237535.html
> Sent from the Pharo Smalltalk mailing list archive at Nabble.com.
>
>



--
Best regards,
Igor Stasenko AKA sig.

Reply | Threaded
Open this post in threaded view
|

Re: First commit to Pharoinbox. Need advice.

Henrik Sperre Johansen
In reply to this post by HwaJong Oh
On 26.01.2011 08:13, HwaJong Oh wrote:
> My last package is Balloon-hjo.72.mcz.
> It is small testing&  refactoring on Ballloon supporting collections.
>
> I wonder this type of work is acceptable?
>
> Best Regards
Refactorings and more tests are certainly welcome!

Here's a few comments:
While I like keeping common functionality in one place, I'm not sure
creating a new helper object for each at:/at:put: access is a good idea.
Nor keeping the helper cached to avoid this. Have you considered using a
Trait instead? To me it seems like a good use-case for one.

  To the tests:
- setUp uses at:put:. testAt asserts the values set in setUp. Thus, is
it not really just another at:put: test? Does it test something not
covered by  the other at:put: test(s)?
- testAtPutLargeInteger uses a static integer. This is not ideal, as the
range of SmallInteger depends on platform (I realize though, that it is
large enough for it not to fail on 64bit as well).
Using f.ex SmallInteger maxVal + staticInteger is more intention revealing.
Although, max/minVal actually returns a constant at the moment rather
than a platform-specific value, and thus would fail on 64bit. I'd
consider this a bug which it'd be nice if a test exposed :)
- testWriteXYEndianOn uses 256*256*256. etc. Probably down to
preference, but I think they'd looks nicer using the bitShift: (or #<<
if you prefer less parenthesis) operator.

Cheers,
Henry

Reply | Threaded
Open this post in threaded view
|

Re: First commit to Pharoinbox. Need advice.

HwaJong Oh
Thank you for your detailed advice.
Although i could only comply bitShift: adivce. Using trait was unsuccessful. super send is not possible with traits and applying PointArrayTrait on ShortPointArray makes strange error.

New version is out. (Balloon-hjo.73)
Reply | Threaded
Open this post in threaded view
|

Re: First commit to Pharoinbox. Need advice.

HwaJong Oh
In reply to this post by Igor Stasenko
At least my tests are passing, so minimum quality is guarrentied. Wonder what the quality tester want. :-)

Is this where i should go?  http://code.google.com/p/pharo/issues/list
Reply | Threaded
Open this post in threaded view
|

Re: First commit to Pharoinbox. Need advice.

Igor Stasenko
On 26 January 2011 12:47, HwaJong Oh <[hidden email]> wrote:
>
> At least my tests are passing, so minimum quality is guarrentied. Wonder what
> the quality tester want. :-)
>
Anything which you would actually want to see being on his place :)

> Is this where i should go?  http://code.google.com/p/pharo/issues/list

Yes.

> --
> View this message in context: http://forum.world.st/First-commit-to-Pharoinbox-Need-advice-tp3237535p3237883.html
> Sent from the Pharo Smalltalk mailing list archive at Nabble.com.
>
>



--
Best regards,
Igor Stasenko AKA sig.

Reply | Threaded
Open this post in threaded view
|

Re: First commit to Pharoinbox. Need advice.

Henrik Sperre Johansen
In reply to this post by HwaJong Oh
On 26.01.2011 12:40, HwaJong Oh wrote:
> Thank you for your detailed advice.
> Although i could only comply bitShift: adivce. Using trait was unsuccessful.
> super send is not possible with traits and applying PointArrayTrait on
> ShortPointArray makes strange error.
>
> New version is out. (Balloon-hjo.73)
Strange, using a trait worked fine when I tried it, all tests passing,
see v74 in inbox for details.

Cheers,
Henry


Reply | Threaded
Open this post in threaded view
|

Re: First commit to Pharoinbox. Need advice.

Stéphane Ducasse
In reply to this post by HwaJong Oh

On Jan 26, 2011, at 12:40 PM, HwaJong Oh wrote:

>
> Thank you for your detailed advice.
> Although i could only comply bitShift: adivce. Using trait was unsuccessful.
> super send is not possible with traits and applying PointArrayTrait on
> ShortPointArray makes strange error.

super send are possible with traits.
Tell us what was your problem.

Stef

>
> New version is out. (Balloon-hjo.73)
> --
> View this message in context: http://forum.world.st/First-commit-to-Pharoinbox-Need-advice-tp3237535p3237870.html
> Sent from the Pharo Smalltalk mailing list archive at Nabble.com.
>


Reply | Threaded
Open this post in threaded view
|

Re: First commit to Pharoinbox. Need advice.

Stéphane Ducasse
In reply to this post by Henrik Sperre Johansen
+1

> Refactorings and more tests are certainly welcome!
>
> Here's a few comments:
> While I like keeping common functionality in one place, I'm not sure creating a new helper object for each at:/at:put: access is a good idea. Nor keeping the helper cached to avoid this. Have you considered using a Trait instead? To me it seems like a good use-case for one.
>
> To the tests:
> - setUp uses at:put:. testAt asserts the values set in setUp. Thus, is it not really just another at:put: test? Does it test something not covered by  the other at:put: test(s)?
> - testAtPutLargeInteger uses a static integer. This is not ideal, as the range of SmallInteger depends on platform (I realize though, that it is large enough for it not to fail on 64bit as well).
> Using f.ex SmallInteger maxVal + staticInteger is more intention revealing.
> Although, max/minVal actually returns a constant at the moment rather than a platform-specific value, and thus would fail on 64bit. I'd consider this a bug which it'd be nice if a test exposed :)
> - testWriteXYEndianOn uses 256*256*256. etc. Probably down to preference, but I think they'd looks nicer using the bitShift: (or #<< if you prefer less parenthesis) operator.
>
> Cheers,
> Henry
>


Reply | Threaded
Open this post in threaded view
|

Re: First commit to Pharoinbox. Need advice.

HwaJong Oh
In reply to this post by Stéphane Ducasse
Yes, you are right. I was wrong. There is nothing super send. I had error editing my trait codes around "super", so i thought trait can super send.  
 http://screencast.com/t/yyjLLm0S

MoveMethod:

FriendOfPointArray >>
at: index

        ^(pointArray superAt: index * 2 - 1) @ (pointArray superAt: index * 2)

to

PointArrayTrait >>
at: index

        ^(super at: index * 2 - 1) @ (super at: index * 2)


while editing "pointArray superAt:" to "super at:", the editor made error.

Copying the code to native Mac editor, edit them, then copy back to Pharo editor suceeded.

Reply | Threaded
Open this post in threaded view
|

Re: First commit to Pharoinbox. Need advice.

Igor Stasenko
On 27 January 2011 07:28, HwaJong Oh <[hidden email]> wrote:

>
> Yes, you are right. I was wrong. There is nothing super send. I had error
> editing my trait codes around "super", so i thought trait can super send.
>  http://screencast.com/t/yyjLLm0S
>
> MoveMethod:
>
> FriendOfPointArray >>
> at: index
>
>        ^(pointArray superAt: index * 2 - 1) @ (pointArray superAt: index * 2)
>
> to
>
> PointArrayTrait >>
> at: index
>
>        ^(super at: index * 2 - 1) @ (super at: index * 2)
>
>
> while editing "pointArray superAt:" to "super at:", the editor made error.
>

Hehe.. funny mistake :)

Btw, what is this FriendOfPointArray? The name sounds strange

> Copying the code to native Mac editor, edit them, then copy back to Pharo
> editor suceeded.
>
>
> --
> View this message in context: http://forum.world.st/First-commit-to-Pharoinbox-Need-advice-tp3237535p3241521.html
> Sent from the Pharo Smalltalk mailing list archive at Nabble.com.
>
>



--
Best regards,
Igor Stasenko AKA sig.

Reply | Threaded
Open this post in threaded view
|

Re: First commit to Pharoinbox. Need advice.

Henrik Sperre Johansen
On 27.01.2011 17:38, Igor Stasenko wrote:

> On 27 January 2011 07:28, HwaJong Oh<[hidden email]>  wrote:
>> Yes, you are right. I was wrong. There is nothing super send. I had error
>> editing my trait codes around "super", so i thought trait can super send.
>>   http://screencast.com/t/yyjLLm0S
>>
>> MoveMethod:
>>
>> FriendOfPointArray>>
>> at: index
>>
>>         ^(pointArray superAt: index * 2 - 1) @ (pointArray superAt: index * 2)
>>
>> to
>>
>> PointArrayTrait>>
>> at: index
>>
>>         ^(super at: index * 2 - 1) @ (super at: index * 2)
>>
>>
>> while editing "pointArray superAt:" to "super at:", the editor made error.
>>
> Hehe.. funny mistake :)
Yeah, I created an issue for it:
http://code.google.com/p/pharo/issues/detail?id=3603
> Btw, what is this FriendOfPointArray? The name sounds strange
>
It actually has a class comment :)
"Removes duplication of PointArray & ShortPointArray. They do not share
same superclass, therefore this class is to remove to duplicated codes
by extracting class"

Hence my suggestion of a trait to keep it in rather than creating a new
helper instance for each at: or at:put:

Cheers,
Henry