Edgewall Software
Modify

Opened 11 years ago

Closed 11 years ago

#7770 closed enhancement (fixed)

Modularize trac-admin

Reported by: Remy Blank Owned by: Remy Blank
Priority: normal Milestone: 0.12
Component: admin/console Version: 0.12dev
Severity: normal Keywords:
Cc: Branch:
Release Notes:
API Changes:

Description

All trac-admin commands are implemented in trac/admin/console.py. As suggested by Christian in comment:11:ticket:7723, it would be nice to modularize it by introducing an IAdminCommandProvider. The advantages would be:

  • Make it easier to write commands.
  • Allow plugins to add commands to trac-admin.
  • Minimize code duplication for command parsing and auto-completion.
  • Move code into the individual modules, where it belongs. This would also allow console and web admin modules to use common code.

I have started working on this, and the first patch below is a proposal for the interface, for the command manager, and the changes to be done for one set of commands (the config commands).

I don't think that it would be necessary to work on this in a branch, as the new interface can be introduced without breaking backward compatibility, and commands can be migrated to modules progressively.

Comments very welcome.

Attachments (3)

7770-modularize-trac-admin-r7647.patch (13.4 KB ) - added by Remy Blank 11 years ago.
Interface and command manager proposal, and working example for config commands
7770-modularize-trac-admin-2-r7649.patch (46.8 KB ) - added by Remy Blank 11 years ago.
Updated patch also moving most of the ticket commands to ticket/admin.py
7770-modularize-trac-admin-r7656.patch (95.2 KB ) - added by Remy Blank 11 years ago.
Complete patch against trunk

Download all attachments as: .zip

Change History (9)

by Remy Blank, 11 years ago

Interface and command manager proposal, and working example for config commands

comment:1 by osimons, 11 years ago

A good initiative - and much discussed over the years without much progress. Thanks for picking it up :-)

I haven't tried the patch yet, just read through it. I see that the interface only has one get method, and returns the method to call for each command. No doubt that works well, but it just feels slightly different to the usual way of interfaces that commonly have a get to 'register' and a process method to 'handle' the execution. I'm fine with either though.

in reply to:  1 comment:2 by Remy Blank, 11 years ago

Replying to osimons:

A good initiative - and much discussed over the years without much progress.

I should have searched the trac-dev archives before starting. I'll do that later today, but if you have any pointers to interesting threads on the subject, I'm all ears.

I haven't tried the patch yet, just read through it. I see that the interface only has one get method, and returns the method to call for each command. No doubt that works well, but it just feels slightly different to the usual way of interfaces that commonly have a get to 'register' and a process method to 'handle' the execution.

The reason I prefer the former is that it doesn't require duplicating the dispatching code in every process method. It's also the model used for IWikiSyntaxProvider, IIRC.

Thanks for your comments!

by Remy Blank, 11 years ago

Updated patch also moving most of the ticket commands to ticket/admin.py

by Remy Blank, 11 years ago

Complete patch against trunk

comment:3 by Remy Blank, 11 years ago

Keywords: review added

The patch above should be complete:

  • All commands except help and initenv have been moved into individual modules. The latter is somewhat special, as there's no environment yet on execution, so it can't be placed into a Component. It could still be moved into a function in e.g. trac/env.py, if desired.
  • All commands have been tested, with all combinations of optional arguments.
  • Auto-completion for all commands has been tested.

At this point, I'm happy with the result. It would be great if I could get some review (and possibly some more testing) before I apply the patch.

In a second step, I'd like to add more unit tests, for the commands that are not yet tested.

comment:4 by osimons, 11 years ago

Patch tested, and it works really well - at least for the handful of commands I tested. Auto-completions is really nice too :-)

The structure of the code also looks good. Reading through the patch it looks like the new providers are all implemented in the logical places in the code.

+1 on committing it. Well done!

comment:5 by Remy Blank, 11 years ago

Keywords: review removed

Thanks for the review. The patch has been committed in [7677].

I'll leave this ticket open as a reminder for me to add more unit tests for the various trac-admin commands.

comment:6 by Remy Blank, 11 years ago

Resolution: fixed
Status: newclosed

Adding more unit tests should actually be tracked in a new ticket. Closing this one, as it's done.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Remy Blank.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Remy Blank to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.