rabbitmq-dotnet-client
rabbitmq-dotnet-client copied to clipboard
rabbitmq
Reduce public API as much as possible
#739 Was my first PR to reduce the public API surface, but I believe there are still more things that make sense to internalize. I'll be looking at those in a subsequent PR.
@bording thanks a lot for #739. I'm hoping to get an RC 1 of version 6 published by the end of March. Should I wait for a follow-up API pull request?
I did want to make one more pass and see if there was any more cleanup that was worth doing, but I just haven't had the spare time to do it yet. Do you know more specifically when you'll be trying to get RC1 out?
I'll try and get it done before then.
I'm not going to rush anyone gracious enough to volunteer their time. How about by the end of March? There are still a couple issues related to bugs in the 6.0.0 milestone I need to investigate.
#739, #763 and #783 appear to have addressed most of this. I reviewed the API approval file to see if anything else seemed like obvious candidates for removal.
I'll leave this open until the EOD 3/31 if @bording or @stebet have input.
#739, #763 and #783 appear to have addressed most of this. I reviewed the API approval file to see if anything else seemed like obvious candidates for removal.
I do think that I've already cleaned up the obvious stuff. I think there's further opportunity here to be more intentional about the remaining API surface, but that is more about making actual design changes vs. just quickly internalizing things.
An example of what I'm talking about: ITcpClient is a public interface that currently must be public because there is an extensibility point on ConnectionFactoryBase that requires it.
I think there are a few places like this where I don't see that there is much value in allowing for that level of customization, so you could remove things like that and clean up the API.
However, I don't really expect the kind of discussions needed for those kinds of design changes to be feasible in the 6.0 time frame.
It probably makes sense to move that kind of thing to 7.0, since I suspect there are going to be larger API changes required there for the kind of things @stebet has been talking about, for example.
RabbitMQ.Client.Framing.Constants appears to have been one of the types made non-public for 6.0.0, but it is specifically referenced by the API documentation for ShutdownEventArgs.ReplyCode. Are there plans to make it public again, replace it with something else, or neither?
In either of the latter two cases, the API documentation should be updated.
@danlyons-softek thanks for noticing. Please file that as a new issue in this repository instead of as a comment here.
@danlyons-softek we can introduce a new namespace, RabbitMQ.Client.Constants, or make the existing one public again. You are welcome to submit a PR that does the latter :)
@bording FYI.
That's a bad design to expose the ushort value like that and then need a completely separate, unrelated constants class like to interpret it.
The public ReplyCode should be an enum that's self-describing.