refactor!: remove unnecessary exported.ChannelI interface #5703
Conversation
colin-axner
added
the
type: code hygiene
| @@ -166,14 +166,6 @@ func (ClientState) VerifyConnectionState( | |||
| panic(errors.New("legacy solo machine is deprecated")) | |||
| } | |||
|
|
|||
| // VerifyChannelState panics! | |||
| func (ClientState) VerifyChannelState( | |||
There was a problem hiding this comment.
there are more stale funcs in this file that can be removed
colin-axner
requested review from
AdityaSripal,
crodriguezvega,
charleenfei,
damiannolan,
chatton,
DimitrisJim and
srdtrk
as code owners
| - No relevant changes were made in this release. | ||
| ### API removals | ||
|
|
||
| The `exported.ChannelI` interface has been removed. Please use the concrete type. |
There was a problem hiding this comment.
What is your thinking on removal v deprecation? Do we flat out remove when we consider highly unlikely someone using it? (which is most definitely case here)
Would be nice to have a depreciation policy.
There was a problem hiding this comment.
My approach is to deprecate changes which are disruptive to users. Users shouldn't be using the exported.ChannelI interface (it was never intended to be used externally), as they likely need to cast to channeltypes.Channel, but if they are, the only diffs they have is changing from exported.ChannelI to channeltypes.Channel
I agree a deprecation policy would be nice, but I feel it might be good to give ourselves some flexibility in deciding on a case by case basis, as some removals are more disruptive than others
There was a problem hiding this comment.
yup, makes sense. I think a deprecation policy would be nice to mention that and also enforce a timeline (i.e deprecations last for one major release) considering we have a number of deprecated things that seem to still be lurking around.
There was a problem hiding this comment.
Some other points that come to mind:
- these changes can be applied pre-v9 (users can make the changes in a pr before bumping to v9, so the depreciation notice could actually be posted in the v8 release, but honestly don't think anyone will notice until the compiler breaks)
- no functional changes
I feel those are 2 good points for justifying immediate removal. Maybe we can start bundling justifications in a document so it can be more formalized
| GetVersion() string | ||
| ValidateBasic() error | ||
| } | ||
|
|
||
| // CounterpartyChannelI defines the standard interface for a channel end's | ||
| // counterparty. | ||
| type CounterpartyChannelI interface { |
There was a problem hiding this comment.
wonder if its possible to remove this also?
There was a problem hiding this comment.
I think so, decided to tackle one interface at a time. I suspect some other interfaces can be removed as well
Description
a feel good pr
removes the unncecessary interface exported.ChanelI. It shouldn't exist since we only have a single concrete implementation of Channel. It was likely originally added to avoid import cycles, but this obviously isn't an issue
closes: #XXXX
Commit Message / Changelog Entry
see the guidelines for commit messages. (view raw markdown for examples)
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/).godoccomments.Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.