Edgewall Software
Modify

Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#8867 closed defect (fixed)

_shref_formatter() no longer called in a class derived from Formatter

Reported by: jouvin@… Owned by: Christian Boos
Priority: normal Milestone: 0.12
Component: wiki system Version: 0.8.1
Severity: trivial Keywords:
Cc: Branch:
Release Notes:
API Changes:

Description

Hi,

Redirect plugin is no longer working in recent revisions of 0.12. This happened between r7799 and r8858. This plugin defines a class RedirectFormatter that inherits from Formatter and redefines method _shref_formatter. The RedirectFormatter version of this method is no longer called.

Any change in Trac core that may explain this? Could somebody point me to some documentation?

Thanks in advance.

Michel

Attachments (0)

Change History (10)

comment:1 by anonymous, 10 years ago

In fact, the change was easy… but undocumented (at least I have not been able to find it!). Formatter now calls shref2_formatter() instead of _shref_formatter(). Basically the same function, except that the match groups have the '2' suffix too.

Probably worth an entry in the API 0.12 changes.

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

Replying to anonymous:

Probably worth an entry in the API 0.12 changes.

We could do that. OTOH, methods starting with an underscore are "protected" by convention, and hence are not part of the public API.

comment:3 by Christian Boos, 10 years ago

Component: generalwiki system
Owner: set to Christian Boos
Severity: normaltrivial
Version: none0.8.1

True, that was an undocumented change because it was a _... method. But I should have nevertheless sticked to the rule to not make an API change unless it can't be avoided. I'll fix that by swapping shref/shref2 again.

in reply to:  3 comment:4 by anonymous, 10 years ago

Replying to cboos:

True, that was an undocumented change because it was a _... method. But I should have nevertheless sticked to the rule to not make an API change unless it can't be avoided. I'll fix that by swapping shref/shref2 again.

It's up to you, I don't know if the reason was good enough for the change. I personnally don't have any pb with the change, the fix was trivial for the Redirect plugin: I did the same as you did in formatter.py, add a new _shref2_formatter() method. You are right this is some sort of private method but there are some cases where this is handy to override it…

comment:5 by anonymous, 10 years ago

I'd add that the most important IMO is to allow to maintain the backward compatibility in plugins. The current solution (with a new method name) does allow this very easily. In particular if you change the match group name (as this is done currently in _shref2_formatter) it's really better to provide a new method name.

comment:6 by anonymous, 10 years ago

Interesting side issue…!!! I tried to change the Version associated with the ticket for 0.8.1 to 0.12dev and I got the following error:

Submission rejected as potential spam (Akismet says content is spam)

in reply to:  5 comment:7 by Christian Boos, 10 years ago

Status: newassigned

Replying to anonymous:

I'd add that the most important IMO is to allow to maintain the backward compatibility in plugins.

Right, except that plugin authors should expect some breakage when they are using private methods (those starting with _...). If they think they really need to do so and want some sort of stability, they have to tell us about this and we make the methods public, changing the contract…

In particular if you change the match group name (as this is done currently in _shref2_formatter) it's really better to provide a new method name.

Except that I could have kept the old group names as well…

comment:8 by Christian Boos, 10 years ago

Resolution: fixed
Status: assignedclosed

Fixed in r8959. Sorry Michel, you get to fix the plugin again ;-)

comment:9 by Michel Jouvin <jouvin@…>, 10 years ago

Thanks, I'll try to do it. I have to check if I need to revert back to __shref or use the new __shrefbr…

I basically agree with your remarks in principle. Just for clarification, I am not the plugin author, just happened to be the first to try to use it with 0.12…!!! I don't know Trac API enough to be able to say if there is a better way to achieve the same result without using private methods. And Redirect plugin can probably be replaced by ServerRedirect but I have no idea if this other plugin has the same need. And I cannot say if Redirect is the only plugin to do such a thing, in which case this is probably not worth upgrading the method to public or if there are several other plugins with the same concerns… In fact I found the explanation in Bitten tickets so Redirect is clearly not the only one…

comment:10 by thomas.moschny@…, 10 years ago

As far as I can see, the TracRedirect plugin now works again, without need to patch it. Thanks for the fix!

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 as closed 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.