JEventProcessorPODIO: deprecate podio:output_include_collections parameter

[veprbl]

This is a follow up on a resolution from #1323

What kind of change does this PR introduce?

• Bug fix (issue #__)
• New feature (issue #__)
• Documentation update
• Other: __

Please check if this PR fulfills the following:

• Tests for the changes have been added
• Documentation has been added / updated
• Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

No

Does this PR change default behavior?

No

[github-actions]

Capybara summary for PR 1466

• rec_dis_10x100_minQ2=0_craterlake
• rec_dis_10x100_minQ2=1000_craterlake
• rec_dis_18x275_minQ2=0_craterlake
• rec_dis_18x275_minQ2=1000_craterlake
• rec_dis_5x41_minQ2=0_craterlake
• rec_e_1GeV_20GeV_craterlake
• rec_pi_1GeV_20GeV_craterlake
  ^{_{Last updated 2024-06-03T18:15-04:00 a80b057}}

[wdconinc]

Been experimenting with automated code review. This is what chatgpt gives.

Here is a review of the provided patch:

General Overview

The patch deprecates the podio:output_include_collections parameter in favor of podio:output_collections and makes related code adjustments. It also includes some changes for "Include What You Use" (IWYU) compliance.

Detailed Review

1. Deprecation of podio:output_include_collections

Changes Made:

• Introduced a new parameter podio:output_collections.
• Added logic to handle the deprecated podio:output_include_collections parameter, which clears output_collections if the deprecated parameter is used.
• Modified logging and processing functions to handle the new parameter.
• Added warnings and delays if the deprecated parameter is used.

Code Review:

• Consistency: The parameter deprecation is handled consistently throughout the code.
• Backward Compatibility: The code provides backward compatibility with a clear deprecation warning and a delay, ensuring users notice the change.
• Logging: The error logging for the deprecated parameter is helpful but using m_log->warn instead of m_log->error might be more appropriate since it's not an error in the system but a usage warning.

2. Include What You Use (IWYU) Adjustments

Changes Made:

• Adjusted the order of includes for IWYU compliance.
• Added an entry to .github/iwyu.imp to correctly map <bits/chrono.h> to <chrono>.
• Removed unnecessary IWYU pragma comments.

Code Review:

• Correctness: The adjustments seem correct and improve the compliance with IWYU guidelines.
• Maintenance: These changes improve maintainability by ensuring only necessary headers are included.

Suggested Improvements

1. Warning Level: Change the log level for deprecation warning to warn:

   m_log->warn("The podio:output_include_collections was provided, but is deprecated. Use podio:output_collections instead.");

2. Code Comments: Add comments explaining why the delay is introduced when the deprecated parameter is used. This will help future maintainers understand the reasoning behind this decision.

Summary

Overall, the patch is well-constructed, handling the deprecation gracefully while maintaining backward compatibility. The IWYU changes are appropriate and follow best practices. Only minor adjustments regarding logging levels and additional comments are recommended.

Example Modified Section

if (m_output_include_collections_set) {
    m_log->warn("The podio:output_include_collections was provided, but is deprecated. Use podio:output_collections instead.");
    // Adding a delay to ensure users notice the deprecation warning.
    using namespace std::chrono_literals;
    std::this_thread::sleep_for(10s);
}

Great job on the patch!

[veprbl]

@wdconinc Ready to approve?

[wdconinc]

Looks good.