Re: [PR] LOG4J2-2073: Fix NPE in ConfigurationScheduler when scheduling cron tasks [logging-log4j2]

["vy (via GitHub)" <gi...@apache.org>]

vy commented on code in PR #3941:
URL: https://github.com/apache/logging-log4j2/pull/3941#discussion_r2401591012


##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationScheduler.java:
##########


Review Comment:
   1. Remove the `FIX (part 1)`-sort comments, please. Instead, if something is not obvious at first glance, explain in the rationale, _when necessary_.
   2. Run linter: `./mvnw spotless:apply -pl :log4j-core`
   3. Don't touch/change unrelated lines



##########
log4j-core/src/test/java/org/apache/logging/log4j/core/config/CronSchedulerNpeTest.java:
##########


Review Comment:
   Would you implement the following changes, please?
   
   1. This test is not effective. That is, it even passes without the fix. Could you make sure this test fails without the fix?
   2. Could you move this test from `log4j-core/src/test` to `log4j-core-test/src/test`?
   3. Could you add the license header?
   4. Could you make sure `./mvnw verify -pl :log4j-core,:log4j-core-test` passes before submitting your changes? 



-- 
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: notifications-unsubscribe@logging.apache.org

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