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

[jouvin@…]

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

[anonymous]

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.

[Remy Blank]

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.

[Christian Boos]

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.

[anonymous]

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…

[anonymous]

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.

[anonymous]

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)

[Christian Boos]

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…

[Christian Boos]

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

[Michel Jouvin <jouvin@…>]

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…

[thomas.moschny@…]

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