Wrong type about consuldotnet HOT 6 CLOSED

Sure, I guess this is a reasonable change. Are you currently blocked by this though?

Feel free to send a PR!

from consuldotnet.

Whenever we accept any changes like this, we need to consider its binary compatibility.
I think here, the change would be binary incompatible. So on one hand I get the argument that we shouldn't use concrete types on our public interfaces, but on the other hand I think we should just prioritise binary compatibility.

from consuldotnet.

this caused problems for a short time when i wanted to mock it in a unit test. but then i found a solution around it and the problem doesn't block me anymore. i just wanted to point out that there could be advantages to use IDictionary there. unfortunately i don't know what is meant by binary compatibility.

i'll gladly do a pr. if binary compatibility is more important, then you can just decline it.

from consuldotnet.

There are also other occurrences in properties:

• https://github.com/G-Research/consuldotnet/blob/master/Consul/Agent.cs#L152
• https://github.com/G-Research/consuldotnet/blob/master/Consul/Agent.cs#L255
• https://github.com/G-Research/consuldotnet/blob/master/Consul/AuthMethod.cs#L37
• https://github.com/G-Research/consuldotnet/blob/master/Consul/Client_Request.cs#L54
• https://github.com/G-Research/consuldotnet/blob/master/Consul/Semaphore.cs#L223
• https://github.com/G-Research/consuldotnet/blob/master/Consul/Utilities/Extensions.cs#L35

And as types in functions:

• https://github.com/G-Research/consuldotnet/blob/master/Consul/Agent.cs#L328
• https://github.com/G-Research/consuldotnet/blob/master/Consul/Agent.cs#L330
• https://github.com/G-Research/consuldotnet/blob/master/Consul/AuthMethod.cs#L54
• https://github.com/G-Research/consuldotnet/blob/master/Consul/AuthMethod.cs#L59
• https://github.com/G-Research/consuldotnet/blob/master/Consul/Client_Request.cs#L80

And in interfaces:

• https://github.com/G-Research/consuldotnet/blob/master/Consul/Interfaces/IAgentEndpoint.cs#L52
• https://github.com/G-Research/consuldotnet/blob/master/Consul/Interfaces/IAgentEndpoint.cs#L53

from consuldotnet.

I've made two branches: "master" that contains the changes only in Catalog.cs, and "more" that contains all changes.

please let me know if i should make a pr from any of those branches.

from consuldotnet.

unfortunately i don't know what is meant by binary compatibility.

By binary compatibility I mean the compatibility when at runtime an assembly references different version of an assembly than was used to compile that particular assembly. It is possible that some library can be compiled against Consul 1.6.10.7, but at runtime it will be using higher version (e.g. Consul 1.6.10.8). If the newer version is not binary compatible with older version, we will get runtime errors like this one:

Unhandled exception. System.MissingMethodException: Method not found: 'System.Threading.Tasks.Task`1<Consul.QueryResult`1<System.Collections.Generic.Dictionary`2<System.String,System.String[]>>> Consul.ICatalogEndpoint.Services(System.Threading.CancellationToken)'.
   at ConsulTesting.Program.Main()
   at System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start[TStateMachine](TStateMachine& stateMachine)
   at ConsulTesting.Program.Main()
   at ConsulTesting.Program.<Main>()

In small solutions it is probably not an issue, as all projects are going to be compiled against the same version. Unfortunately, the larger the particular solution is, the more likely some projects or NuGet packages will use older version of Consul - and it will lead to fatal errors at runtime.

from consuldotnet.