Interfaces to be implemented by users should not have unexported methods

[Aneurysm9]

In #4668 I expressed concern that a new interface was created that unnecessarily included an unexported method with the goal of preventing direct implementation of the interface. Instead, providing a "partial" implementation to an initializer was required, with the initializer wrapping the partial implementation in an unexported struct that had an implementation of the unexported interface method with an empty body. However, since the interface had only one real method and that is the method provided to the initializer, it seemed to serve no purpose and only added complexity.

The response to this concern was that adding the unexported interface method was in conformance with practice throughout this repo and must remain for consistency. There appears to be no formal statement of this practice or of a recommendation that it be used. It is also not used consistently throughout this repository and attempting to do so would likely have significant downstream impact.

Of the 69 exported interfaces I found in this repository only 13 include an unexported method, including the internalinterface.InternalInterface that was created for this purpose but only used directly in one exported interface (and indirectly in five more that embed that exported interface). One of those interfaces (configmapprovider.Retrieved) was only recently changed to include an unexported method in #4577, with no explanation offered for why.

component.Factory (through embedding internalinterface.InternalInterface)
component.ExporterFactory (through embedding component.Factory)
component.ExtensionFactory (through embedding component.Factory)
component.ProcessorFactory (through embedding component.Factory)
component.ReceiverFactory (through embedding component.Factory)
experimental/component.ConfigSourceFactory (through embedding component.Factory)
config.Exporter
config.Extension
config.Processor
config.Receiver
configmapprovider.Retrieved
consumertest.Consumer
internalinterface.InternalInterface

I think that we should generally discourage the inclusion of unexported methods in exported interfaces and have a high bar for justifying the use of the pattern.

[bogdandrutu]

First of all, please stop the personal accusations (because I know that is the case in things like One of those interfaces (configmapprovider.Retrieved) was only recently changed to include an unexported method in #4577, with no explanation offered for why.).

One of those interfaces (configmapprovider.Retrieved) was only recently changed to include an unexported method in #4577, with no explanation offered for why.

The explanation is very simple, once a pattern was adopted by the project we can use that without having to explain every time we use that patter "why".

[Aneurysm9]

The explanation is very simple, once a pattern was adopted by the project we can use that without having to explain every time we use that patter "why".

No explanation was offered at the time the change was made and none is offered now. "Because I can" is not an explanation.

I think that some explanation is required when changing an interface that could be implemented by external code to one that cannot, regardless whether that pattern was in use elsewhere in the codebase. It may be that it made sense to do it elsewhere but not in that case. Nobody was given a chance to discuss that, however.

[bogdandrutu]

No explanation was offered at the time the change was made and none is offered now. "Because I can" is not an explanation.

I don't see where I wrote "because I can", but looks like you continue to attack me, and blame me... Maybe I can write this again to you: "The decision was made when we added that pattern first time, reusing the pattern does not need another explanation of why we adopted that pattern".

[bogdandrutu]

The initial decision was to do that, because we tried to split one of the factories to see how it would be to have a different factory for logs (as an example), and we could not get the code to look good at all, since we wanted to have only one config per factory. If you want to prove that the initial decision was wrong, consider to split one of the factory and use it in tests/config/etc., and see where you get.

[bogdandrutu]

To prove @Aneurysm9 intentions are just to blame me and they don't care about the project, look at this issue related to exactly this problem, here is the timeline:

1. I created the issue for discussing the pattern to use Allow Factories to support new signals #3921
2. The issue was assign by @alolita to @Aneurysm9, who did not do any work, did not provide feedback in the issue, or any activity related to that issue.
3. @mx-psi picked the issue, then started to work on it. One of the PRs Remove BaseProcessorFactory, force using helper #4175 was even approved by @Aneurysm9 where we had the plan in the description to force implementation to use a helper via embedding an internal interface with a private func internalinterface.InternalInterface.

[Aneurysm9]

To prove @Aneurysm9 intentions are just to blame me and they don't care about the project, look at this issue related to exactly this problem, here is the timeline:

You prove nothing of the sort and do yourself no favors by ascribing motives to me that I do not have. I'm sure that if you were to look you could find other things in the project that may have occupied my time during September.

[Aneurysm9]

I think that we should generally discourage the inclusion of unexported methods in exported interfaces and have a high bar for justifying the use of the pattern.

I don't see where I wrote anything indicating that I don't feel that the *Factory interfaces meet that bar. I want there to be a reasoned approach taken and guidance that can be given to developers and reviewers so that there can be consistency and we have a basis for assessing whether and when to deviate from that guidance. Right now it appears that decisions regarding whether interfaces should be implementable outside of their package is at best arbitrary.