https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43548 --- Comment #1 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 206080 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206080&action=edit Bug 43548: Convert runreport.pl to the Getopt::Long::Descriptive convention Converts runreport.pl's GetOptions call to Koha::Script->describe_options (bug 43546). No option here is genuinely mandatory -- --format defaults to 'text', --to/--from/--email default from KohaAdminEmailAddress, and the script's real requirement (at least one saved report ID) is a positional argument, which Getopt::Long::Descriptive can't express as a named option. This surfaced gaps in the bug 43546 convention, now added to Koha::Script->describe_options: - runreport.pl previously had --help (brief), --man (full Pod::Usage manual, since its POD is substantial), and --help+--verbose (also full manual). The new convention's --help covers the first case, but had no equivalent to --man. Rather than reinvent that per-script, --man is now added automatically alongside --help: it prints the calling script's own POD in full (Pod::Usage -verbose 2, defaulting to $0) and exits. --help+--verbose no longer triggers the full manual; --verbose reverts to its own, unrelated meaning of execution verbosity. Scripts using describe_options get --man for free going forward. - A positional-argument requirement (like this script's report ID(s)) has no per-option 'required' flag to declare it with, so it was initially left as manual post-parse validation. That is exactly the kind of ad hoc, unenforced, unparseable check bug 43546 exists to replace for named options -- it just had nicer error formatting. describe_options now accepts a declarative 'args' key (in the same trailing hashref as 'epilog'), e.g. C<args => { min => 1, name => 'reportID', variadic => 1 }>, enforced by describe_options itself with the same consistent error/usage output as a missing required option. This also gives static tooling (e.g. the koha-plugin-crontab plugin, which reads script source rather than executing it) a literal marker for a required positional argument, the same way it already reads 'required => 1' for named options. describe_options also returns ($opt, $usage) in list context (unchanged in scalar context) for any post-parse validation 'args' doesn't cover. Beyond the mechanical conversion, a few of runreport.pl's own option- handling clauses were simplified using more of Getopt::Long::Descriptive's existing constraint vocabulary, with no behaviour change: - --to and --from now declare { implies => 'email' }, so GLD itself sets $opt->email when either is given, replacing the manual "if ($to or $from or $send_email) { $send_email = 1 }" check. - The --separator/--quote "only meaningful with --format csv" warnings stay as manual post-parse checks -- GLD's constraints are pass/fail (a failing one aborts the script via $usage->die), so turning this warn-and-continue behaviour into a declarative constraint would make it a hard failure instead, which is a real behaviour change. They're simplified to flat checks against $opt->separator/$opt->quote directly, dropping the now-unneeded mutable local copies. - The "unless ($format) { assume 'text' }" fallback is dropped, since { default => 'text' } on the format option already guarantees this (it was only reachable via an explicit --format '', which behaves differently now, but nobody relies on that). - The positional report IDs are copied to @report_ids once, right after describe_options, instead of using @ARGV directly throughout -- purely a readability nicety pairing the declared 'args' name with a named variable at the point of use. Test plan: 1. Confirm the dependency (Getopt::Long::Descriptive) and helper (Koha::Script->describe_options) are already in place from bug 43546. 2. Run: misc/cronjobs/runreport.pl --help Confirm it lists all documented options, the reportID argument, and usage examples, and exits 0. 3. Run: misc/cronjobs/runreport.pl --man Confirm it prints the script's full existing POD (NAME, SYNOPSIS, OPTIONS, DESCRIPTION, USAGE EXAMPLES, etc.) and exits 0. 4. Run: misc/cronjobs/runreport.pl Confirm it fails immediately with "ERROR: At least 1 reportID argument required (got 0)" and the usage text, before any database access, and exits non-zero. 5. Create one or more saved SQL reports (Reports > Guided reports), note their IDs, then run: misc/cronjobs/runreport.pl <id> misc/cronjobs/runreport.pl --format csv --csv-header <id> misc/cronjobs/runreport.pl --verbose <id> <id2> misc/cronjobs/runreport.pl --separator ';' <id> Confirm output matches this script's behavior before this patch: tab-separated by default, comma-separated with a header row when requested, verbose logging of the SQL/argument count/result count, and a "Cannot specify separator if not using CSV format" warning (with default tab-separated output still produced) for the last case. 6. Run misc/cronjobs/cart_to_shelf.pl --help and --hours 24 (bug 43546) and confirm both are unaffected by the describe_options changes here. 7. Confirm koha-qa.pl passes for the changed files. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43548 --- Comment #2 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 206082 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206082&action=edit Bug 43548: Add --man and positional-argument support to Koha::Script->describe_options Converting runreport.pl (next commit) surfaced two gaps in the describe_options convention introduced by bug 43546: - runreport.pl previously had --help (brief), --man (full Pod::Usage manual, since its POD is substantial), and --help+--verbose (also full manual). The new convention's --help covers the first case, but had no equivalent to --man. Rather than reinvent that per-script, --man is now added automatically alongside --help: it prints the calling script's own POD in full (Pod::Usage -verbose 2, defaulting to $0) and exits. --help+--verbose no longer triggers the full manual; --verbose reverts to its own, unrelated meaning of execution verbosity. Scripts using describe_options get --man for free going forward. - A positional-argument requirement (like runreport.pl's report ID(s)) has no per-option 'required' flag to declare it with. Left as manual post-parse validation, it's exactly the kind of ad hoc, unenforced, unparseable check bug 43546 exists to replace for named options -- it would just have nicer error formatting. describe_options now accepts a declarative 'args' key (in the same trailing hashref as 'epilog'), e.g. C<args =\> { min =\> 1, name =\> 'reportID', variadic =\> 1 }>, enforced by describe_options itself with the same consistent error/usage output as a missing required option. This also gives static tooling (e.g. the koha-plugin-crontab plugin, which reads script source rather than executing it) a literal marker for a required positional argument, the same way it already reads 'required =\> 1' for named options. describe_options also returns ($opt, $usage) in list context (unchanged in scalar context) for any post-parse validation 'args' doesn't cover. Test plan: 1. prove t/Koha/Script.t 2. koha-qa.pl passes for Koha/Script.pm (--man and args are exercised functionally in the next commit, which converts runreport.pl to use them) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43548 --- Comment #4 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 206084 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206084&action=edit Bug 43548: Simplify runreport.pl's option-handling using Getopt::Long::Descriptive A few of runreport.pl's own option-handling clauses can lean on more of Getopt::Long::Descriptive's existing constraint vocabulary, with no behaviour change: - --to and --from now declare { implies => 'email' }, so GLD itself sets $opt->email when either is given, replacing the manual "if ($to or $from or $send_email) { $send_email = 1 }" check. - The --separator/--quote "only meaningful with --format csv" warnings stay as manual post-parse checks -- GLD's constraints are pass/fail (a failing one aborts the script via $usage->die), so turning this warn-and-continue behaviour into a declarative constraint would make it a hard failure instead, which would be a real behaviour change. They're simplified to flat checks against $opt->separator/$opt->quote directly, dropping the now-unneeded mutable local copies. - The "unless ($format) { assume 'text' }" fallback is dropped, since { default => 'text' } on the format option already guarantees this (it was only reachable via an explicit --format '', which behaves differently now, but nobody relies on that). - The positional report IDs are copied to @report_ids once, right after describe_options, instead of using @ARGV directly throughout -- purely a readability nicety pairing the declared 'args' name with a named variable at the point of use. Test plan: 1. Run misc/cronjobs/runreport.pl --separator ';' <id> (without --format csv) and confirm it still warns "Cannot specify separator if not using CSV format" and still produces default tab-separated output, unaffected by the warning. 2. Run misc/cronjobs/runreport.pl --format csv --separator ';' --quote "'" --csv-header <id> and confirm the custom separator/quote are applied. 3. Run misc/cronjobs/runreport.pl --to someone@example.org <id> and confirm it still attempts to email the report (implied by --to). 4. Run misc/cronjobs/runreport.pl <id> <id2> and confirm both reports run. 5. Confirm koha-qa.pl passes for the changed file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43548 --- Comment #5 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 206088 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=206088&action=edit Bug 43548: Convert runreport.pl to the Getopt::Long::Descriptive convention Converts runreport.pl's GetOptions call to Koha::Script->describe_options (bug 43546), using the --man and args support added in the previous commit. No option here is genuinely mandatory -- --format defaults to 'text', --to/--from/--email default from KohaAdminEmailAddress, and the script's real requirement (at least one saved report ID) is a positional argument, declared via C<args => { min => 1, name => 'reportID', variadic => 1 }> rather than a manual "unless (@ARGV) { ... die }" check. Test plan: 1. Run: misc/cronjobs/runreport.pl --help Confirm it lists all documented options, the reportID argument, and usage examples, and exits 0. 2. Run: misc/cronjobs/runreport.pl --man Confirm it prints the script's full existing POD (NAME, SYNOPSIS, OPTIONS, DESCRIPTION, USAGE EXAMPLES, etc.) and exits 0. 3. Run: misc/cronjobs/runreport.pl Confirm it fails immediately with "ERROR: At least 1 reportID argument required (got 0)" and the usage text, before any database access, and exits non-zero. 4. Create a saved SQL report (Reports > Guided reports), note its ID, then run: misc/cronjobs/runreport.pl <id> misc/cronjobs/runreport.pl --format csv --csv-header <id> misc/cronjobs/runreport.pl --verbose <id> Confirm output matches this script's behavior before this patch (tab-separated by default, comma-separated with a header row when requested, and verbose logging of the SQL/argument count/result count). 5. Run misc/cronjobs/cart_to_shelf.pl --help and --hours 24 (bug 43546) and confirm both are unaffected. 6. Confirm koha-qa.pl passes for the changed files. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43548 --- Comment #7 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Obsoleting this patch: the Koha::Script->describe_options --man/args support has moved to bug 43546 (it's part of establishing the convention itself, and other sibling bugs depending on 43546 need it too, not just this one). See bug 43546 comments for the updated patch. This bug's remaining two patches (runreport.pl conversion + follow-up) now depend on 43546's updated series instead of including this commit directly. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43548 Alex Carver [Acerock7] <alex@rcls.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43548 --- Comment #8 from Alex Carver [Acerock7] <alex@rcls.org> --- Created attachment 207232 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207232&action=edit Bug 43548: Convert runreport.pl to the Getopt::Long::Descriptive convention Converts runreport.pl's GetOptions call to Koha::Script->describe_options (bug 43546), using the --man and args support added in the previous commit. No option here is genuinely mandatory -- --format defaults to 'text', --to/--from/--email default from KohaAdminEmailAddress, and the script's real requirement (at least one saved report ID) is a positional argument, declared via C<args => { min => 1, name => 'reportID', variadic => 1 }> rather than a manual "unless (@ARGV) { ... die }" check. Test plan: 1. Run: misc/cronjobs/runreport.pl --help Confirm it lists all documented options, the reportID argument, and usage examples, and exits 0. 2. Run: misc/cronjobs/runreport.pl --man Confirm it prints the script's full existing POD (NAME, SYNOPSIS, OPTIONS, DESCRIPTION, USAGE EXAMPLES, etc.) and exits 0. 3. Run: misc/cronjobs/runreport.pl Confirm it fails immediately with "ERROR: At least 1 reportID argument required (got 0)" and the usage text, before any database access, and exits non-zero. 4. Create a saved SQL report (Reports > Guided reports), note its ID, then run: misc/cronjobs/runreport.pl <id> misc/cronjobs/runreport.pl --format csv --csv-header <id> misc/cronjobs/runreport.pl --verbose <id> Confirm output matches this script's behavior before this patch (tab-separated by default, comma-separated with a header row when requested, and verbose logging of the SQL/argument count/result count). 5. Run misc/cronjobs/cart_to_shelf.pl --help and --hours 24 (bug 43546) and confirm both are unaffected. 6. Confirm koha-qa.pl passes for the changed files. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Alex Carver [Acerock7] <alex@rcls.org> -- You are receiving this mail because: You are watching all bug changes.