Deprecate incorrectly named API #424
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Provide new names that don't offend golint - Use new names throughout the code base - Mark the old names deprecated and redefine constants using new values
- Provide new names that don't offend golint - Use new names throughout the code base - Mark the old names deprecated and redefine constants using new values
- Provide new names that don't offend golint - Use new names throughout the code base - Mark the old names deprecated and redefine constants using new values
- Provide new names that don't offend golint - Use new names throughout the code base - Mark the old names deprecated and redefine constants using new values
This renaming is more involved that other ones. Interfaces are hard.
First of all, introduce a new interface with identifiers that do not
offend golint:
- use "Callbacks" instead of "SessionCallbacks" to avoid stuttering
when importing (it will be called "session.Callbacks" in user code)
- use "GetPublicKeyID" method name instead of "GetPublicKeyId"
Then update all existing code to use the new interface. Secure Session
itself should call the new method, accept and store Callbacks instance.
Users of Secure Session should implement the new GetPublicKeyID method.
However, in order to maintain backwards compatibility we have to allow
the old code to continue using the old SessionCallbacks interface with
GetPublicKeyId method. We implement this using an adapter struct that
translates new method calls into the old ones. Since Go does not have
method overloading we have to accept an "interface {}" instance and
check its type dynamically. These compatibility shims can be removed
when we drop the old interface.
- Provide new name that don't offend golint - Use the new name throughout the code base - Mark the old method deprecated and reimplement it using the new one
All these dynamic checks in session.New() warrant a test which verifies that we actually can handle the old interface values and reject invalid ones. This test can be removed together with the old SessionCallbacks API.
ilammy
added
W-GoThemis 🐹
Lagovas
reviewed
Mar 12, 2019
It turns out that CGo actually does export C defines as Go constants, therefore we don't need to use intermediate variables for that. Previous code actually does not compile with Go 1.9.2 that we have on CI, but it is fine with my Go 1.12. The new code is fine in both environments.
|
Uh... @vixentael, @Lagovas, you know what? I've just refreshed Go Report Card for Themis with the current master that does not include this PR. It seems that we're A-OK with the current names and don't have to rename anything. I'm not really sure why. Should we proceed with this PR then? |
|
imho will be better to update constants according to golang conventions but leave as is callback's interface as is for now |
|
💯 agree with @Lagovas, let's leave callbacks as is |
|
Okay then. I have reverted the two commits touching the callbacks. Now we only rename the constants and that one method of Secure Session. |
Lagovas
approved these changes
Mar 13, 2019
vixentael
approved these changes
Mar 13, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PRs #422, #423 have fixed up miscellaneous golint warnings. Some of them are for incorrectly named constants and methods. This PR renames the affected identifiers into something acceptable by golint while simultaneously deprecating the old names.
We cannot remove the old names immediately due to backwards compatibility concerns. They will be actually removed in some later release, once all existing users have migrated to the new API. But for now we keep compatibility shims of various complexity to allow usage of the old API.
Summary of changes:
cell.CELL_MODE_*constantscompare.COMPARE_*constantskeys.KEYTYPE_*constantssession.STATE_*constantssession.SecureSession.GetRemoteIdmethodRenaming constants is actually very easy:
This is true for methods as well.
Renamesession.SessionCallbacksinterfaceCompatibility test for old Secure Session API(Edit: these two commits were removed later during code review.)
This renaming is more involved that other ones. Interfaces are hard.
First of all, introduce a new interface with identifiers that do not offend golint:
Callbacksinstead ofSessionCallbacksto avoid stuttering when importing (it will be calledsession.Callbacksin user code)GetPublicKeyIDmethod name instead ofGetPublicKeyIdThen update all existing code to use the new interface. Secure Session itself should call the new method, accept and store
Callbacksinstance. Users of Secure Session should implement the newGetPublicKeyIDmethod.However, in order to maintain backwards compatibility we have to allow the old code to continue using the old
SessionCallbacksinterface withGetPublicKeyIdmethod. We implement this using an adapter struct that translates new method calls into the old ones. Since Go does not have method overloading we have to accept aninterface {}instance and check its type dynamically. These compatibility shims can be removed when we drop the old interface.All these dynamic checks in
session.New()warrant a test which verifies that we actually can handle the old interface values and reject invalid ones. This test can be removed together with the old SessionCallbacks API.