Claude-at-Instaclustr commented on pull request #258: URL: https://github.com/apache/commons-collections/pull/258#issuecomment-1038389673
I think I have fixed the issues you noted. However, I am fighting with formatting and getting it all just right. I should be able to check it in tomorrow evening UTC. Claude On 13/02/2022 00:07, Alex Herbert wrote: > > Thanks for the updates. > > Currently the CI build does not check code coverage, so you have to do > this locally. > > Can you look at test coverage again: > > |mvn test jacoco:report open > target/site/jacoco/org.apache.commons.collections4.bloomfilter/index.html | > > You have some code that is picked up by missing coverage. Looking at > the code some of these cases really should be covered and relate to > operations performed on filters of different sizes. A few points: > > * The code at SparseBloomFilter Line 221 is just weird; it looks > like testing code. > * Some constructors in the BloomFilters are not used at all. > * The SimpleBloomFilter is catching IOOB exception and rethrowing > it. Is this necessary? I would let the IOOB exception trickle up. > If you want to catch it then at least test this works and raises > an IAE rather than a IOOBE. > * In SimpleBloomFilter.forEachBitMap is the bitMap ever null? > * No coverage that the |makePredicate| in BitMapProducer will send > |0L| to the LongBiFunction when the array is exhausted. Note the > documentation for makePredicate uses |apply| when it should be > using |forEachBitMap|. This needs a better example that can > actually be compiled. The method should better explain the usage: > "Create a single long argument predicate to be passed to the > |forEachMethod| of a second BitMapProducer. For each |long| passed > to the predicate from the second BitMapProducer a matching |long| > from this BitMapProducer will be paired with it and passed to the > argument LongBiFunction. The |long| from this producer will be the > first argument to the BiFunction; when the bitmaps are exhausted > from this producer then the value 0 is substituted until the > second producer is also exhausted". In this use case the > LongBiFunction should be named a LongBinaryPredicate as it returns > a boolean if using the |java.util.function| naming conventions. > > So there's a few things to fix up and I'll do a full review of what we > now have in the coming few days. > > — > Reply to this email directly, view it on GitHub > <https://github.com/apache/commons-collections/pull/258#issuecomment-1037596846>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AVM5ARNJ62JMEWECS5CQLODU23Y2LANCNFSM5FUBJMFA>. > Triage notifications on the go with GitHub Mobile for iOS > <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> > or Android > <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. > > You are receiving this because you commented.Message ID: > ***@***.***> > -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
