> thanks for the report. > Mariano so far we did not change any of the rules provided by RB. For now we > will run small lint and collect information. > > > Ok, I understand. But since I am SmallLintying my packages I guess that my > feedback does not hurt for future work (when you do start changing rules).
Indeed. > Some more feedback: > > 6) RBAbstractClassRule is confusing. "Checks for references to classes that > have subclassResponsibility methods. Such references might be creating > instances of the abstract class or more commonly being used as the argument > to an isKindOf: message which is considered bad style." In my case, it found > FLBenchmarkRunner because it has INSTANCE side messages with > subclassResponsability. However, all references to the class > FLBenchmarkRunner send class side messages to it. So.... maybe It can > exclude class side messages that do not end up sending #new or #basicNew? > > 7) how does the Manifiesto class envolves while the real packages are changed > (class and methods are renamed or removed) ? > If a method class is removed then I guess it is easy to remove the metadada > associated to them. But what about if a method change? you cannot be sure if > the rule exclusion still make sense or not. So, I guess you should remove the > metadata as well ? > > 8) RBMethodHasNoTimeStampRule seems to have a problem with Traits because it > shows me lots of methods without timestamp and they all result to come from a > trait. > > 9) Sometimes, clicking on a method raises an error. Find attached > PhaorDebug.log > > 10) RBSubclassResponsibilityNotDefinedRule could show me WHICH are the leaf > subclasses that do NOT implement the method ... otherwise it is complicated > to find it. > > 11) I don't like storing the Manifiest class in the default/main package. I > changed ManifiestFuel to be in Fuel-SmallLint rather than Fuel but then, as > soon as I run the rules again, it moves back the class to Fuel. Can't it > check if the class already exist in Smalltalk globals and if true just use it > and don't move it ? > > 12) It would be nice to group rules by criteria. I saw you have #group in > each rule so I guess it is a matter of time to have the UI aware of that, > right? > > > Cheers > > > > Stef > > > > > Hi guys. After our meeting at Lille, I did a pass of SmallLint over Fuel > > project. I have to say that I enjoyed the tool and that. I love the > > manifiesto idea. I love to store metada and to be able to reject rules and > > scenarios to avoid having false positives again in the future. In addition, > > it looks like I had less false positive than with other tools. However, > > there were a couple of rules that I don't understand or may suggest > > something: > > > > 1) First, apart form showing the package between parenthesis, I would also > > show the protocol (category). Because some rules play with that, so it is > > handy if you can directly see them there, otherwise you need to browse the > > code. > > > > 2) RBContainsRule is not clear for me. " 'Checks for the common code > > fragment: "(aCollection detect: [:each | ''some condition''] ifNone: [nil]) > > ~= nil". contains: can simplify this code to "aCollection contains: [:each > > | ''some condition'']". Not only is the contains: variant shorter, it > > better signifies what the code is doing.'" > > That is not very true. #contains answers true or false, while > > detect:ifNone:[] answers whatever, and in your example it is nil. So, the > > answer will be different and hence it is not the same. Ok, in your example, > > you compare after with == nil... but... I don't really see a use case. For > > example, in my case it detected: > > > > classNamed: className > > > > ^ (migrations > > detect: [:m | m sourceClassName = className ] > > ifNone: [ ^ self globalClassNamed: className ]) > > targetClass. > > > > > > 3) RBLawOfDemeterRule: maybe you can exclude string concatenation? (#,) > > because it detected a lot of messages like this: > > signalWith: aGlobalName and: aSelector > > ^ self signal: 'Method ', aGlobalName, '>>#', aSelector, ' not found.' > > > > Also, what about excluding messages like #asXXX. For example, here it > > detected "self signature asByteArray". Is that completly breaking the law > > of demeter? mmm I don't think so. > > > > verifySignatureFrom: aDecoder > > > > | streamSignature | > > streamSignature := ByteArray new: self signature size. > > aDecoder nextEncodedBytesInto: streamSignature. > > (self signature asByteArray = streamSignature) ifFalse: [ > > FLBadSignature > > signalCurrentSignature: self signature > > streamSignature: streamSignature ]. > > > > > > 4) RBSentNotImplementedRule: I didn't understand why it has selected: > > > > largeIdentityHash > > > > <primitive: 75> > > self primitiveFailed > > > > both messages are implemented :( > > > > > > 5) RBClassInstVarNotInitializedRule could be smarter and search not only > > for #initialize but for #initializeXXX. Lots of times, I initialize my > > objects with data, using a particular nitialize method, such as: > > initializeWithBehaviors: someBehaviors extensionMethods: > > someExtensionMethods > > so the rule says there is no initialize...but I have that one ;) > > > > > > That's all the feedback so far. It helped me to find some things to improve > > so thanks! > > > > > > -- > > Mariano > > http://marianopeck.wordpress.com > > > > > > > > -- > Mariano > http://marianopeck.wordpress.com > > <PharoDebug.log>
