[Pharo-project] First commit to Pharoinbox. Need advice.
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 -- View this message in context: http://forum.world.st/First-commit-to-Pharoinbox-Need-advice-tp3237535p32375... Sent from the Pharo Smalltalk mailing list archive at Nabble.com.
On 26 January 2011 08:13, HwaJong Oh <daliot.oh@gmail.com> 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-tp3237535p32375... Sent from the Pharo Smalltalk mailing list archive at Nabble.com.
-- Best regards, Igor Stasenko AKA sig.
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 -- View this message in context: http://forum.world.st/First-commit-to-Pharoinbox-Need-advice-tp3237535p32378... Sent from the Pharo Smalltalk mailing list archive at Nabble.com.
On 26 January 2011 12:47, HwaJong Oh <daliot.oh@gmail.com> 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-tp3237535p32378... Sent from the Pharo Smalltalk mailing list archive at Nabble.com.
-- Best regards, Igor Stasenko AKA sig.
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
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) -- View this message in context: http://forum.world.st/First-commit-to-Pharoinbox-Need-advice-tp3237535p32378... Sent from the Pharo Smalltalk mailing list archive at Nabble.com.
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
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-tp3237535p32378... Sent from the Pharo Smalltalk mailing list archive at Nabble.com.
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. -- View this message in context: http://forum.world.st/First-commit-to-Pharoinbox-Need-advice-tp3237535p32415... Sent from the Pharo Smalltalk mailing list archive at Nabble.com.
On 27 January 2011 07:28, HwaJong Oh <daliot.oh@gmail.com> 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-tp3237535p32415... Sent from the Pharo Smalltalk mailing list archive at Nabble.com.
-- Best regards, Igor Stasenko AKA sig.
On 27.01.2011 17:38, Igor Stasenko wrote:
On 27 January 2011 07:28, HwaJong Oh<daliot.oh@gmail.com> 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
+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
participants (4)
-
Henrik Sperre Johansen -
HwaJong Oh -
Igor Stasenko -
Stéphane Ducasse