Ticket Options like default_milestone should be in the TicketSystem not the TicketModule

[Felix Schwarz <felix.schwarz@…>]

These don't have anything to do with the request or the web page. Also you can see that you need these options in the TicketSystem so that the information where/how the settings are stored in the configuration is currently duplicated.

[Christian Boos]

I'm fine with the move of options from TicketModule to TicketSystem.

However, I think both the config API and the access through descriptors have their use, one should pick whichever makes for the most readable code.

So for example:

trac/ticket/default_workflow.py

@@ -287 +287 @@
                                name=resolutions[0]))
             else:
                 id = 'action_%s_resolve_resolution' % action
-                selected_option = req.args.get(id,
-                        self.config.get('ticket', 'default_resolution'))
+                selected_option = req.args.get(id, 
+                        TicketSystem(self.env).default_resolution)
                 control.append(tag(['as ', tag.select(
                     [tag.option(x, selected=(x == selected_option or None))
                      for x in resolutions],

is fine, but:

trac/ticket/model.py

@@ -75 +75 @@
                 # Ignore for new - only change through workflow
                 pass
             elif not field.get('custom'):
-                default = self.env.config.get('ticket',
-                                              'default_' + field['name'])
+                default = getattr(TicketSystem(self.env), 'default_' + field['name'], None)
             else:
                 default = field.get('value')
                 options = field.get('options')

makes it actually harder to read, I think.

[Remy Blank]

Replying to cboos:

I'm fine with the move of options from TicketModule to TicketSystem.

Won't this unnecessarily break plugins that expect those options in TicketModule? Shouldn't we at least have "forwarders" in TicketModule and deprecate their use?

[Felix Schwarz <felix.schwarz@…>]

Replying to rblank:

Won't this unnecessarily break plugins that expect those options in TicketModule? Shouldn't we at least have "forwarders" in TicketModule and deprecate their use?

So the idea would be to add properties which forward to the TicketSystem + DeprecationWarning?

[Remy Blank]

Replying to Felix Schwarz <felix.schwarz@…>:

So the idea would be to add properties which forward to the TicketSystem + DeprecationWarning?

I'd say yes for the properties, but DeprecationWarnings are ugly. I would just add a big fat comment above the properties warning that they will be removed in 0.13.

[Christian Boos]

Move default_... field value Options from TicketModule to TicketSystem. (patch on trunk, r8787)

[Christian Boos]

New patch 8716_move_ticket_options_to_ticketsystem.patch​ added, please review.

[Remy Blank]

Looks good, one minor nitpick: I would use a single _warnings attribute with a set() of the option names where the warning has been shown, instead of using separate _warn_* attributes and getattr() magic.

[Christian Boos]

Patch applied, with the above suggestion taken into account, in [8792].

Thank you Felix for the initial patch.

[Christian Boos]

Ah yes, and as discussed in comment:1, use config.get when it's more elegant than accessing the Option directly (r8793).

[Remy Blank]

Replying to cboos:

Patch applied, with the above suggestion taken into account, in [8792].

You could even put name directly into the set(), instead of "_warn_" + name (i.e. warn_flag is obsolete) :-)

[Christian Boos]

Replying to rblank:

You could even put name directly into the set(),

Done in [8800], thanks!

[Ryan J Ollos]

The deprecated accessor in the TicketModule was removed in [12962] (comment:18:ticket:11494).