Plugin#onModule methods are deprecated and final

[javanna]

Plugin#onModule methods are deprecated as they have been replaced by specific plugin classes that need to be extended depending on the type of plugin and the needed functionalities. In the past those onModule methods were called via reflection and received some specific guice module as an argument and allowed to do many things with it, probably too many.

Now that there is a replacement, it makes sense to deprecate these onModule methods, but I don't get why we would leave them behind, make them final, hence unusable by plugins, and deprecate them at the same time. Either they should be usable but deprecated, or they should be gone. Making them final is the same as removing them I think? What we have now just makse people that used them angry I think :) Or maybe there are reasons why this was done that I am missing, not sure.

[nik9000]

Now that there is a replacement, it makes sense to deprecate these onModule methods, but I don't get why we would leave them behind, make them final, hence unusable by plugins, and deprecate them at the same time.

I like the way we have it because you get a compiler error in your plugin as soon as you try to compile it against 5.0. I imagine you'll get dozens, but at least this one points you right to trying to override a final method and when you look at the method it tells you what to do instead. I think technically this is the wrong way to use deprecated because, like you say, if you mark something as deprecated it should continue to work for a version or so. But we don't have that kind of stability for plugins. In this case deprecated just gives us a convenient space to stick a string. I think it'd be more correct to remove @Deprecated and keep the final method with the warnings in the javadoc. But I think what we have is fine.

I haven't read the specification for the @Deprecation annotation in a while so I could be totally wrong.

[s1monw]

@javanna I think there is one little detail that isn't obvious. Today if you implement a onModule(SomeModule foo) you don't actually override anything. The super class doesn't implement those methods. Guice applies some magic to invoke these methods, if you have a typo in there nothing happens. Now we defined them as final to ensure users that implement them get a compilation error. I wonder if we should add a comment to this class explaining that?

[nik9000]

Guice applies some magic to invoke these methods

That magic is all home grown.

I wonder if we should add a comment to this class explaining that?

I think @deprecated implement {@link ActionPlugin} instead is pretty descriptive. I'd be happy to stick some more words in the class level Javadoc to make it more clear though.

[javanna]

yea the problems caused by using reflection are clear to me. If this was all done on purpose I am ok with it. It's just that finding the methods deprecated seems like a joke, cause they are effectively gone :) We could have been nicer and only deprecate those methods in 5.0 and remove them in 6.0, but I know we don't do this for plugins so anything would have upset plugin developers anyways and using deprecated doesn't make a lot of difference after all.

[s1monw]

That magic is all home grown.

dude, I know :) we fork it so I am not blaming anybody upstream

I think @deprecated implement {@link ActionPlugin} instead is pretty descriptive. I'd be happy to stick some more words in the class level Javadoc to make it more clear though.

++

yet I mean we should put some in-line comments why we make this final at all

[javanna]

yet I mean we should put some in-line comments why we make this final at all

++ if somebody doesn't know (like me today) they may think it's a bug or a silly mistake

[nik9000]

Right. I've assigned this to myself and stuck this at the head of my fancy to-do queue (browser tabs).