-
Notifications
You must be signed in to change notification settings - Fork 50
Refactor shim interfaces to not be implementable externally #3100
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
Conversation
|
This change is part of the following stack: Change managed by git-spice. |
faf6286 to
d0ef58b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3100 +/- ##
=======================================
Coverage 68.78% 68.79%
=======================================
Files 335 336 +1
Lines 43560 43575 +15
=======================================
+ Hits 29962 29976 +14
- Misses 11887 11888 +1
Partials 1711 1711 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
d0ef58b to
5fa7b74
Compare
|
Why not just use comments to the purpose? If something out there manages to use these with success, although they are unstable, why be extra strict here and promote additional breakage? That said we are using downcasts aggressively for example on values of type shim.InstanceState so I doubt anything out there is actually going to be broken. |
| @@ -0,0 +1,9 @@ | |||
| package internalinter | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this is public? :) So a determined user could still implement it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No - this is in the internal directory, so no user outside of the bridge can use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay !
|
I'd rather we get ahead of it and just prevent any external implementations. We are then completely free to add methods to these interfaces. I do agree there are unlikely to be any. |
|
Since there's likely zero impact I'll approve. If we hear from users we can reconsider and back out. |
This PR is a pure refactor. The
shiminterfaces are not intended to be implemented externally. They change often and due to the fact they are public we have kept accruing new variants to keep the public interface backwards compatible which add unnecessary complexity.This PR adds
InternalInterfaceinterface with a private method which is only implemented byinternalinter.Internal. This can be embedded in interfaces which are not intended to be implemented outside of the bridge. Implementers in the bridge can embed the implementation ofInternalInterfacebut external users can not.This means that we are free to add methods to interfaces which are not intended to be implemented externally. This will in turn allow us to delete some of the
SchemaandResourcevariants undershim.