Attached is a patch which I'm requesting feedback on. Some notes on the changes:

Renamed method-scope function render_wiki_to_html_without_req to render_wiki_to_html. The long name seems unnecessary and makes it difficult to limit the width of expressions to 80 chars.

The comment variable is used for both normal comments and formatted attachment descriptions, since an attachment description is effectively a comment on that attachment. Since you can't have a normal comment and attachment within the same change there is no conflict. I felt like this was better than adding another entry to the data dictionary.

I'll rework the patch based on feedback if any of these changes seem undesirable.

I'm working on adding a unit test, but ran into a some issues. I'll follow-up on that tomorrow.

I added a unit test, and in the process of getting the unit test working, discovered some other issues that the attached patch addresses:

The unit test for TicketFormatter.format was failing with TemplateNotFound: Template "ticket_email_mimic.html" not found (full traceback below). TicketFormatter and WikiFormatter were not implementing ITemplateProvider, so they must have been replying on the AnnouncerPreferences and SubscriptionManagementPanel making the templates available, since those are the only two components in the plugin that implement ITemplateProvider. I suspect this would lead to an error in the case that the admin chose to not enable the preferences panel. With this patch, TicketFormatter and WikiFormatter implement ITemplateProvider and can make the necessary templates available without relying on other components being enabled.

Renamed tests/formatter.py to tests/formatters.py since the test module name should match name of the module under test.

Other minor changes to the test harness.

The expected output for the attachment notification HTML is contained in a text file. Maybe there is a better way?

My changes are getting sufficiently complex that I plan to move to developing from a BitBucket fork following completion of work on this patch.

Yes, why now? I'm working on a Mercurial repository clone of t-h.o SVN repo, what is similar to the standard (hg) setup with BitBucket.

Regarding your latest patch, I'd rather prefer to implement ITemplateProvider for only one Component per plugin, make it an abstract one and inherit classes from there as required. I'll add a suitable implementation in announcer.api tomorrow, so you can re-base your changes from there, please.

+1 for the test module name change.

What reason do you see for the changes to announcer/tests/__init__.py? I'm especially curious since I've see a pattern with extra import statements i.e. in TagsPlugin before as well, but couldn't figure out, why it would be preferable over the simpler code here.

Other (test) changes look good to me too, and I do especially appreciate your work on the test harness, because by mow both of us know the tremendous hidden value of it.

I'm still not comfortable enough with Hg to be working effectively, but I think it's time I bite the bullet and take some short term loses in favor of long term efficiency gains. I could just work locally with an Hg clone I suppose. Recently I completed a patch to the Trac core, working from a Git fork in GitHub, and it was very valuable to get some direct code review from Christian via comments on the changeset in GitHub. So I'd like to both move to DVCS, and have a place where you can directly comment on my proposed changes if you like (more on that when I reply to your email).

Regarding your latest patch, I'd rather prefer to implement ITemplateProvider for only one Component per plugin,

I agree, that seems like a better way to go.

I'll add a suitable implementation in announcer.api tomorrow, so you can re-base your changes from there, please.

I'll wait for your changes and rebase.

What reason do you see for the changes to announcer/tests/__init__.py? I'm especially curious since I've see a pattern with extra import statements i.e. in TagsPlugin before as well, but couldn't figure out, why it would be preferable over the simpler code here.

I'm not sure, I'm just sort of fumbling around here, but I was seeing intermittent failures prior to making that change. To debug, I studied osimons implementation for the TagsPlugin, made changes to setup.py and __init__.py that I saw there, and those changes seemed to fix the problem.

Sorry for the mess in [12349]/[12350]. It seems I was also careless with [12294], as it is not working the way I intended. I'll fix that now by direct commit since the changes are simple, and then attach a more complex patch for review, related to the unit tests.

I've attached t10584-r12354-1.patch​ for review, which adds a unit test for the behavior added in this ticket. Assuming the changes I've made here are correct and necessary, then I'd also suggest moving AnnouncerTemplateProvider to announcer.api, or another "more central" location.

I captured the change that is the main subject of this ticket in [12294]. Did you have in mind other entries for the changelog associated with this ticket, or just not notice that entry had already been made?

I'm heavily over-time tonight and already have to postpone another review-request of yours.

No worries on that ... I have plenty to keep me busy and in no rush to push every change. I still have many changesets to review myself, just to get fully up to speed on your development work from the past week or so.

... then I'd also suggest moving AnnouncerTemplateProvider to announcer.api, or another "more central" location.

... thought I'd still keep in mind that we may want to make this change.

It would be nice if we had an implemented or in review state for tickets that are thought to be fully implemented, but will be left open until the next release. I hesitate to implement this site-wide however, since it might be confusing to other users and maintainers.

I captured the change that is the main subject of this ticket in [12294]. Did you have in mind other entries for the changelog associated with this ticket, or just not notice that entry had already been made?

Right, I did not notice. Your changeset has it in, perfect. Nothing else.

... then I'd also suggest moving AnnouncerTemplateProvider to announcer.api, or another "more central" location.

... thought I'd still keep in mind that we may want to make this change.

Lately I tend to put the template provider into that location, where primary/most extensive use happens, often web_ui.py. One import less, especially, if there's no use for it in api.py. OTOH, it's indisputable part of the plugin API, so anyone would start to look for it in api.py, that's the overall concept. Good thing, the class is abstract, so we may move it at any time, because enabling the component is not an issue (no change to trac.ini's [component] section, right?).

It would be nice if we had an implemented or in review state for tickets that are thought to be fully implemented, but will be left open until the next release. I hesitate to implement this site-wide however, since it might be confusing to other users and maintainers.

Yes, I was thinking this for all of AcctMgr, Tags and Announcer lately. If we made it the last action, why now? Theoretically we could introduce another optional field similar to what version (meaning "implemented in version"), i.e. "Plugin Version:" but this might be confusing too. Keyword review is the least intrusive I can think of, and will start for this plugin, if you agree.

For the other aforementioned plugins I've already prepared release notes, so I have no need for this before next stable release. I would rather adopt this convention afterwards there.

Good thing, the class is abstract, so we may move it at any time, because enabling the component is not an issue (no change to trac.ini's [component] section, right?).

Yeah, it looks that way to me. So should we make the change against this ticket, or would you like to make the change against another ticket? Should I take the initiative, or do you already have plans to do it yourself?

Yes, I was thinking this for all of AcctMgr, Tags and Announcer lately. If we made it the last action, why now? Theoretically we could introduce another optional field similar to what version (meaning "implemented in version"), i.e. "Plugin Version:" but this might be confusing too. Keyword review is the least intrusive I can think of, and will start for this plugin, if you agree.

Yeah, that seems like a good way to go, so that we can run quick queries and see which tickets are ready and which need work. To me, it is primarily an issue when there are so many tickets, such as announcer currently having 100+. Once we get out release 1.0 and kill half or more of the tickets, it will be less of an issue.

Add Comment

This ticket has been modified since you started editing. You should review the
other modifications which have been appended above,
and any conflicts shown in the preview below.
You can nevertheless proceed and submit your changes if you wish so.