Finish the refactoring of /trac

[Christian Boos]

In the 0.9 development period, a lot of refactoring has been going on in order to apply the guidelines described in TracDev/CodingStyle#Namingconventions.

However, there are still a few files that don't comply to those rules. As it's troublesome to simply rename a file to its lower case version (think Windows…), the python source files that have still a capitalized name should be moved in subfolders and down-cased at the same time.

I would propose the following refactoring:

• About.py, Notify.py (the generic part), Search.py, Settings.py and Timeline.py should go in trac.general
• Notify.py (the TicketNotifyEmail class), Milestone.py and Roadmap.py should go in trac.ticket

[Christian Boos]

r2380 took care of the Milestone and Roadmap part of that ticket.

Shall we finish this?

[Jonas Borgström]

Why should Search.py, Settings.py etc. be moved into a trac.general package? Is this package only means to make it "safe" to rename files on win32? Imho. "general" trac modules should go into trac and not trac.general.

It would be nice if every file was named according to out naming convention, but I don't think it's worth moving stuff around to less obvious places just to make it so.

[Christian Boos]

The main motivation for the trac.general was indeed to make it safe to rename thoses files on win32.

OTOH, the files About.py, Search.py, Settings.py and Timeline.py are all "modules", and not pieces of the infrastructure, like the other files in trac are. So it could make sense to move them in a subfolder (be it general or modules). Note that attachment.py could also be moved there, then.

Notify.py could remain in trac until a later refactoring.

In the end, I'm neutral on this one, if there's otherwise consensus that the current situation is good enough for 0.9, I have no problem in post-poning the ticket.

[Christian Boos]

We'll take action on this one after milestone:0.9

(I'm still interested in comments on the above comment, though)

[Christian Boos]

[2799] addressed the renaming of trac/Notify.py, thanks Manu!

Still 4 files to go…

[Matthew Good]

Replying to cboos:

Still 4 files to go…

So we've got About.py, Search.py, Settings.py and Timeline.py. I agree with jonas that it's not really necessary to move these simply to make the names lowercase. I'd support just closing this ticket and if we have a reason to refactor one of the modules (like happened with the notification) we can rename it at that time.

[Christian Boos]

Well, is see no reason to close the ticket before the renames actually happen. This ticket is just a reminder of the "plan", and there's no milestone set which means exactly it will be done when it's done…

[Christian Boos]

r4213 fixed the Timeline.py case.

[Christian Boos]

Nearly there:

• r4261 fixed the trac/Settings.py case
• r4262 fixed the trac/Search.py case
• r4263:4264 aimed to fix the trac/About.py case …

… except that it's never a good idea to change file case in-place, because of Windows. Though Subversion seems to deal better with that on updates (it just workedforme this time with 1.4), it's Python itself which has some trouble with that: if you don't clean up the About.pyc, you'll get this:

...
6:14:57 PM Trac[cache] DEBUG: Checking whether sync with repository is needed
Exception in thread Thread-1 (most likely raised during interpreter shutdown):
Traceback (most recent call last):

(there's no traceback)

So while it's easy to get rid of the crash, I prefer that we avoid the problem altogether by moving this about.py file somewhere else.

trac/admin/about.py seems to be a good candidate, as we're also showing the current configuration settings in the About module, which seems enough "admin" related to warrant inclusion in that module.

In case the idea is to the display the configuration inside the Admin module proper (e.g. by integrating the ​TH:IniAdminPlugin into core), then I'd favor removing the About module completely and replace it by a Wiki page.

[Christopher Lenz]

Well, we recommend people clean out there old install for major upgrades, so is this really so much of a problem? I'd rather we keep the about page a separate module… people are complaining about the pre-installed wiki pages, and we'll need to move them to a separate area (i.e. help-pages) at some point anyway.

[Christian Boos]

The problem is that we already recommend a lot of things for the install, and adding a recommendation for avoiding a problem we would have created and could easily prevent is not optimal. I would have liked to show you the ticket we had for the s/Perm.py/perm.py case, but I can't find it this evening :(

Anyway, I'm sure we'll get bug reports for that, if we keep this as is.

I'm sure this would have been a no-brainer if we'd have a straightforward alternative place where to put this… So what about renaming it to trac/colophon.py? ;)

[Christian Boos]

Replying to myself:

I would have liked to show you the ticket we had for the s/Perm.py/perm.py case, but I can't find it

It just struck me while reading the above: it was the log.py vs. Log.py case ;) See #2533.

[anonymous]

So what about trac/Timeline.py vs. trac/timeline/. Same problem, no?

[Christopher Lenz]

(anonymous was cmlenz)

[Christopher Lenz]

Or trac/db.py → trac/db/, etc.

And just to be clear: yeah, we will get tickets about this. And we will be getting a lot more for 0.11, as we're breaking stuff all over the place (unfortunately).

[Christian Boos]

Replying to cmlenz:

Or trac/db.py → trac/db/, etc.

No, I don't think this was problematic. If you actually had read my comments on #2533, you'd have seen that the problem is the following:

But I was nevertheless able to reproduce the problem: the file trac/log.py will be named trac/Log.py if there's already a file named like that before the installation.

And just to be clear: yeah, we will get tickets about this. And we will be getting a lot more for 0.11, as we're breaking stuff all over the place (unfortunately).

… so why not take action when it's possible? Well, I'm giving up on this, it's not that important and didn't deserve all this discussion, let's move on to something else.