Edgewall Software
Modify

Opened 17 years ago

Closed 16 years ago

#2101 closed task (fixed)

Finish the refactoring of /trac

Reported by: Christian Boos Owned by: Christian Boos
Priority: low Milestone: 0.11
Component: general Version: devel
Severity: normal Keywords:
Cc: Branch:
Release Notes:
API Changes:
Internal Changes:

Description

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

Attachments (0)

Change History (17)

comment:1 by Christian Boos, 17 years ago

Milestone: 0.9

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

Shall we finish this?

comment:2 by Jonas Borgström, 17 years ago

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.

comment:3 by Christian Boos, 17 years ago

Status: newassigned

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.

comment:4 by Christian Boos, 17 years ago

Milestone: 0.9

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

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

comment:5 by Christian Boos, 16 years ago

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

Still 4 files to go…

in reply to:  5 comment:6 by Matthew Good, 16 years ago

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.

comment:7 by Christian Boos, 16 years ago

Priority: highlow

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

comment:8 by Christian Boos, 16 years ago

Status: assignednew

comment:9 by Christian Boos, 16 years ago

r4213 fixed the Timeline.py case.

comment:10 by Christian Boos, 16 years ago

Milestone: 0.11

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.

comment:11 by Christopher Lenz, 16 years ago

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.

comment:12 by Christian Boos, 16 years ago

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? ;)

in reply to:  12 comment:13 by Christian Boos, 16 years ago

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.

comment:14 by anonymous, 16 years ago

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

comment:15 by Christopher Lenz, 16 years ago

(anonymous was cmlenz)

comment:16 by Christopher Lenz, 16 years ago

Or trac/db.pytrac/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).

in reply to:  16 comment:17 by Christian Boos, 16 years ago

Resolution: fixed
Status: newclosed

Replying to cmlenz:

Or trac/db.pytrac/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.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Christian Boos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Christian Boos 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.