Hi, the very nice contribution on GIFReadWriter/animated images (see https://pharo.manuscript.com/f/cases/22137) unfortunately introduced some methods that had no sender, were uncategorized and had unused temporary variables, etc.
Something that is meanwhile shown by our tools and easy to fix. I guess it was some overseen leftover. Unfortunately this also slipped through the integration process as it was merged without an approval from a community reviewer. I dont want to lament too much on this specifc case (especially as I now fixed it in https://pharo.fogbugz.com/f/cases/22426). But after I cleaned the P7 image this spring to not have unused temporaries anymore I was suprised to find such glitches again now. So to me it shows that we need to write more tests to detect violations and keep our achieved quality standards up and also should not solely rely on the review process. Otherwise we need to invest our time in cleaning up our own stuff afterwards again and again. To be specific one can evaluate: CompiledMethod allInstances select: [:m | m ast temporaries anySatisfy: [:x | x binding isUnused ]]. to find unused temps - which would be easy to package up into one of the release test to ensure no unused temp vars are left (the tearDown would require an "ASTCache reset"). The problem is that this simple test would take around 20-30 seconds depending on machine and I do not know if it would be a good idea/good solution to integrate. Especially I'm not sure if it is worth the issue or will kill the CI again and again. What do you think about it? Comments appreciated? Thanks T.
