support "optional" parameters in TracReports

[Christian Boos]

Currently if a report uses a user-defined parameter and that parameter is not defined, a warning is displayed (see e.g. {14}).

In case of complex reports with many user-defined parameters, that's a bit unfortunate as it's very easy to cope with missing values in the SQL query, e.g.

...
WHERE
    ($AUTHOR_ IS '' OR author LIKE $AUTHOR_)
    ...

Original proposal

I propose to consider parameter names ending with "_" to be considered as "optional":

• they don't trigger the warning if left empty
• their name is rendered specially in the Parameters box to show that they're optional (with the example above, it would be: (AUTHOR))

I also made the "foldable" legend use a bigger font size.

Updated proposal

The idea about the trailing "_" was abandoned in favor of specifying the default value for the dynamic variables in the SQL query, inside a SQL comment. See comment:5.

[Christian Boos]

example with two optional parameters and a mandatory one

[Christian Boos]

proposed changes on top of r13350

[Ryan J Ollos]

Tested and works well. One place for a documentation update is TracReports#AdvancedReports:DynamicVariables.

[Peter Suter]

Is there a way to specify the default value for parameters? That would be very useful.

[Christian Boos]

Having the possibility to define a default value would indeed by useful. However I don't see where. In TracIni you'd need to be admin to do that and anyway having to edit the report in two different places is not optimal. In the report itself, there's no obvious place either:

• as a SQL comment? -- REVIEWER_=$USER. Easy enough, but looks a bit hackish.
• as a Wiki parameter? {{{#!param name="REVIEWER_" value="$USER"}}}… Well, it's not implemented yet ;-)

[Peter Suter]

Replying to cboos:

• as a SQL comment? -- REVIEWER_=$USER. Easy enough, but looks a bit hackish.

I'd be happy with that. Also, you could drop the underscore, right? All parameters with such defaults would be optional.

[Peter Suter]

This patch​ implements the simple case -- REVIEWER=psuter. (Supporting -- REVIEWER=$USER sounded easy, but I got stuck preventing -- REVIEWER=%s raising ProgrammingError: Incorrect number of bindings supplied.)

[Christian Boos]

Replying to psuter:

Supporting -- REVIEWER=$USER sounded easy, but I got stuck preventing -- REVIEWER=%s raising ProgrammingError: Incorrect number of bindings supplied.)

Removing the default parameter assignments from the SQL solves that part.

See cboos.git@t11837-default-param-for-report.

[Peter Suter]

Sounds good. But defaults should not replace explicit arguments:

trac/ticket/report.py

@@ -812 +812 @@
 
     def get_default_var_args(self, report_args, sql):
         def extract_default_var(fullmatch):
-            report_args[fullmatch.group(1)] = fullmatch.group(2)
+            report_args.setdefault(fullmatch.group(1), fullmatch.group(2))
         return self.arg_default_re.sub(extract_default_var, sql)
 
     def sql_sub_vars(self, sql, args):

Another option to implement the indirection would be:

defaultval = sub_vars(fullmatch.group(2), report_args)
report_args.setdefault(fullmatch.group(1), defaultval)

[Ryan J Ollos]

Retargeting tickets that I think won't be completed for 1.1.5, but please move back if I'm incorrect.

[Christian Boos]

I'll finish this one in time for 1.2.

[Ryan J Ollos]

Would you like to push the changes this week, or should we retarget it to milestone:1.3.1?

[Christian Boos]

Very busy month for me as well, unfortunately. Yeah, this one shouldn't hold back the release.

[Christian Boos]

Focusing on psuter's solution (comment:5), I removed the special handling of trailing _.

Implemented in r15216.

[Robert Becker <trac.edgewall.org@…>]

Is it possible to backport this feature to the next 1.2.x release? The changeset only affects 3 files, so the risk of conflicts is small. I fear that we would have to wait long for this feature otherwise, as 1.2 already took 4 years to be released.