Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename UseRoutingTopology? #444

Closed
timbussmann opened this issue Oct 6, 2017 · 5 comments · Fixed by #614
Closed

Rename UseRoutingTopology? #444

timbussmann opened this issue Oct 6, 2017 · 5 comments · Fixed by #614

Comments

@timbussmann
Copy link
Contributor

Should UseRoutingTopology be renamed to UseCustomRoutingTopology instead?

The only usage of that API I was able to find on docs is related to a custom routing topology: https://docs.particular.net/transports/rabbitmq/routing-topology#custom-routing-topology but that's not clear from the API as I had the impression it's a general purpose API which allows me to define any of the existing built-in topologies.

@adamralph
Copy link
Member

I'm not sure about this. We have to bear in mind that explicit toplogy choice is now mandatory and we intend to deprecate the current topologies in the 5.x timeframe. That will leave UseRoutingTopology as the only method available to choose the new, built-in, topology unless we introduce a Use{NameToBeDecided}Topology, but I'm not convinced that we should.

@WojcikMike
Copy link
Contributor

As we make using of topology mandatory I would leave the name as is. As now in every transport configuration, UseRoutingTopology call will be present with either built-in or custom topology.

@timbussmann
Copy link
Contributor Author

so you're saying in the future users will write something like: UseRoutingTopology(new ConventionalRoutingTopology()) or how would that look like? Because that's what I was trying to to at first but intellisense didn't show any built-in topology to be used. Only after consulting docs I found the correct way to specify a topology.

@adamralph
Copy link
Member

Ah, you have point @timbussmann. The built in routing topologies are internal therefore UseRoutingTopology cannot be used for the mandatory topology selection. Unless we make the new topology being introduced in #411 public, UseRoutingTopology will only ever be useful for selecting custom routing topologies.

@Particular/rabbitmq-transport-maintainers thoughts?

@bording
Copy link
Member

bording commented Oct 10, 2017

The problem with making either our existing or potential new routing topologies public in order to use them with UseRoutingTopology is that they all have non-default constructors that have to be called.

If you're writing a custom routing topology, then you know what dependencies you have, so you know what to pass in, but we wouldn't want users to have to take on the burden of properly creating the ones we provide in the box.

The reason we currently have separate specific APIs to choose a routing topology is that there are potentially unique configuration settings for each of them.

Thinking about this, I suppose it could make sense to rename UseRoutingTopology to UseCustomRoutingTopology.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants