[GitHub] [commons-collections] Claude-at-Instaclustr commented on pull request #258: Simplify bloom filters

[GitBox]

Claude-at-Instaclustr commented on pull request #258:
URL: 
https://github.com/apache/commons-collections/pull/258#issuecomment-1037441278






-- 
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: issues-unsubscr...@commons.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [commons-collections] Claude-at-Instaclustr commented on pull request #258: Simplify bloom filters

[GitBox]

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 
   > 
,
 
   > or unsubscribe 
   > 
.
   > Triage notifications on the go with GitHub Mobile for iOS 
   > 

 
   > or Android 
   > 
.
 
   >
   > 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: issues-unsubscr...@commons.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [commons-collections] Claude-at-Instaclustr commented on pull request #258: Simplify bloom filters

[GitBox]

Claude-at-Instaclustr commented on pull request #258:
URL: 
https://github.com/apache/commons-collections/pull/258#issuecomment-1037441278


   I think I have managed to fix the branch.
   
   On 12/02/2022 09:40, Alex Herbert wrote:
   >
   > @Claudenw  I have updated the github 
   > build action to run some more checks. This includes javadoc of which 
   > you have a few errors to correct. There are currently a lot of 
   > warnings from the rest of the codebase. But warnings are OK as they 
   > are for missing parameters etc. You have some errors which prevent the 
   > javadoc from building. Can you please rebase on master to collect the 
   > changes to the pom.xml.
   >
   > You can simulate the checks that will occur by just using the default 
   > maven goal: |mvn|
   >
   > —
   > Reply to this email directly, view it on GitHub 
   > 
,
 
   > or unsubscribe 
   > 
.
   > Triage notifications on the go with GitHub Mobile for iOS 
   > 

 
   > or Android 
   > 
.
 
   >
   > 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: issues-unsubscr...@commons.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [commons-collections] Claude-at-Instaclustr commented on pull request #258: Simplify bloom filters

[GitBox]

Claude-at-Instaclustr commented on pull request #258:
URL: 
https://github.com/apache/commons-collections/pull/258#issuecomment-1017711213


   @aherbert , @dota17 , @garydgregory , @kinow
   
   Can I get a review and possible merge for this pull?  It has the changes 
requested and is much cleaner than what is there now.  If there are more things 
to clean up I would like to do it as smaller pull requests.
   
   Thank you


-- 
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: issues-unsubscr...@commons.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [commons-collections] Claude-at-Instaclustr commented on pull request #258: Simplify bloom filters

[GitBox]

Claude-at-Instaclustr commented on pull request #258:
URL: 
https://github.com/apache/commons-collections/pull/258#issuecomment-1013898839


   @aherbert I have added tests to cover all reasonable possibilities.


-- 
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: issues-unsubscr...@commons.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [commons-collections] Claude-at-Instaclustr commented on pull request #258: Simplify bloom filters

[GitBox]

Claude-at-Instaclustr commented on pull request #258:
URL: 
https://github.com/apache/commons-collections/pull/258#issuecomment-994712082


   @aherbert , @dota17 , @garydgregory , @kinow
   
   Can I get a review and possible merge of this pull request?  It is 
significantly simpler than the earlier code and I have been using it in some 
development code.  It stands up well in the Bloom filter paper code, and has 
served well in other development efforts as well.


-- 
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: issues-unsubscr...@commons.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org