Combine Connections and DefaultConnections classes #453
Conversation
The DefaultConnections type could be advertised on a class because changing the type of the 'connections: IConnections' property was not allowed in C#. Hence, all uses of this member needed to be cast. Change the interface to always advertise the concrete class 'Connections', and fold the methods of DefaultConnections in there. They now have a runtime check on the presence of default port and will throw if the Connections class hasn't been initialized properly. This change is breaking for middleware writers, not for direct consumers. This fixes #418.
| @@ -215,7 +215,7 @@ export class DatabaseCluster extends DatabaseClusterRef { | |||
| } | |||
|
|
|||
| this.defaultPortRange = new ec2.TcpPortFromAttribute(this.clusterEndpoint.port); | |||
There was a problem hiding this comment.
Does defaultPortRange really need to be a property now?
There was a problem hiding this comment.
We still do static typing one-way. We still force the destination to have a default port by forcing it to be of type IDefaultConnectable (which exposes this property).
| /** | ||
| * This object is used by peers who don't allow reverse connections | ||
| * | ||
| * It still has an associated connection peer, but that peer does not | ||
| * have any security groups to add connections to. | ||
| * Because Connections is no longer an interface but a concrete class, |
There was a problem hiding this comment.
Nit: This sounds like a code review comment, not something that should be checked in.
There was a problem hiding this comment.
Kind of agree, but the next person coming along in this code base might want some content.
Do you prefer removing it?
It doesn't actually need to be exported, so it becomes private documentation, I can also just do that.
There was a problem hiding this comment.
Ah that wasn't actually true. I would still like to retain it somewhere close to the code, but I agree it doesn't need to be a doc comment. Moved to implementation comment.
The DefaultConnections type could be advertised on a class because
changing the type of the 'connections: IConnections' property was not
allowed in C#. Hence, all uses of this member needed to be cast.
Change the interface to always advertise the concrete class
'Connections', and fold the methods of DefaultConnections in there.
They now have a runtime check on the presence of default port and
will throw if the Connections class hasn't been initialized properly.
This change is breaking for middleware writers, not for direct
consumers.
This fixes #418.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.