-
Notifications
You must be signed in to change notification settings - Fork 50
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
storage: add 'Has' feature. #276
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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.
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.
Is there a reason for putting
Has
here instead ofReadableStorage
? Write-only storages will have to figure out how to satisfy this method.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.
Mostly the fear of ending up with a
Has2
function on the package scope. Avoiding that problem seems to mandate we have some interface on the bottom that's in common to both the base read and the base write interface.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.
The Has2 function you posit seems to require a readable storage anyway. I don't really see a strong need for a predefined Storage interfaces. It seems more useful for the consumer of a storage to define what methods it needs satisfied by a provider that is passed.
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.
I have to agree with Ian. I don't really understand why we want to require WritableStorage to implement
Has
, but notGet
.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.
recap: The holistic design goal I'm keeping aim on is: to consistently tell both users and intermediate API designers that they should be referring to either
ReadableStorage
orWritableStorage
. And nothing else -- because then they should use the package-scope functions -- not the methods -- in order to do all their work (and let those functions take care of all the feature detection for them).Now,
Has
is an operation that a user might want to ask for on either one of them.(And surprisingly, I'd say one is almost more likely to want to ask
Has
on the writable side. To askHas
on the reader might be slightly cheaper than opening the read stream for some implementations; but to askHas
before writing can be significantly cheaper than opening a write stream for some implementations. But I digress; let's suffice to say we want the operation available on either direction.)Now if I want to make a package-scope function for
Has
, just to keep 100% consistent with the messaging to the API user about "use the package-scope functions for all operations"... it turns out to be the one thing so far that's legitimately in common to both the read and the write directions.If golang had parametric polymorphism, I'd write two functions:
... but we do not have parametric polymorphism, so this is not an option.
Then our other options are:
Has
function, and document the inconsistency of the "use the package-scope functions for all operations" rule for users.Has
as methods on both anyway?Has
onReadableStorage
only, then... we make people feature-detectHas
on the writer side, when that's wanted?All of those choices sound worse to me than introducing an interface that's at the bottom of the type graph here.
Introducing an interface type that's at the bottom of the type graph gives us what we need to make the package API consistent and low-friction. Coincidentally,
Has
fits really nicely there.(And in the future: if we find more features that might work on both the read or the write direction (and presumably, are optional extensions), we make those package-scope functions of the style
func Foo(Context, Storage, ...) (...)
too, reusing the same bottom type and doing feature-detection internally from there, to minimize burden to the caller.)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.
(This is sort of like the inverse of #277 . It makes the most sense if you look at how many casts show up when you actually try to use the API.)