adamsaghy commented on PR #6485: URL: https://github.com/apache/fineract/pull/6485#issuecomment-5758479185
@alberto-art3ch It looks good, but would you review the below concerns? - ThreadLocalDateTimeProvider.java:47: clear() removes rather than restoring a previous value. Correct today (re-entrancy is impossible, as above), but if set returned the prior provider and the finally restored it, the class would stay correct without depending on that reasoning. - The holder is static on a singleton, while the choice conceptually belongs to CustomAuditingHandler. A non-static ThreadLocal in a provider instance owned by the handler would keep two handler instances independent — only matters for tests, so take it or leave it. - JpaAuditingHandlerRegistrarTest.java:38 passes null for AnnotationMetadata. It works because the parameter is ignored, but the test silently depends on that; mock(AnnotationMetadata.class) would be more robust. -- 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]
