https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43548
--- Comment #1 from Martin Renvoize (ashimema) <[email protected]> --- 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 <[email protected]> -- You are receiving this mail because: You are watching all bug changes. _______________________________________________ Koha-bugs mailing list -- [email protected] To unsubscribe send an email to [email protected] website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
