Add bufplugin package #3431
Add bufplugin package #3431
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
private/bufpkg/bufplugin/parse.go
Outdated
| @@ -0,0 +1,83 @@ | |||
| // Copyright 2020-2024 Buf Technologies, Inc. | |||
There was a problem hiding this comment.
I thought this stuff was being pulled out to a common bufparse package? Also would likely result in some of errors.go being pulled out (and potentially other stuff).
There was a problem hiding this comment.
I did look at pulling it out into bufparse but reverted in 477557f. I noticed we already had another ParseError type in bufcas so thought copying was more consistent. Will refactor back out to a package.
| } | ||
|
|
||
| // ParsePluginFullName parses a PluginFullName from a string in the form "registry/owner/name". | ||
| func ParsePluginFullName(pluginFullNameString string) (PluginFullName, error) { |
There was a problem hiding this comment.
Assume this would be pulled out to a ParseFullName for example
There was a problem hiding this comment.
This has now been pulled out into bufparse.ParseFullName and returns a type of bufparse.FullName. Updated the PR docs.
There was a problem hiding this comment.
Overall, this change seems very reasonable -- it's more of a refactor to unifying the types and then adding the plugin types. We should discuss whether it makes sense to have digest and commit as shared types instead also. There could even be an argument to be made for Key to be a single interface that bufmodule and bufplugin each implement with its own semantics (e.g. restrictions on digest types, etc.).
| // TODO: this is dumb | ||
| return nil, NewInvalidPathError(format, path) |
There was a problem hiding this comment.
bufparse.ParseRef only returns an error from the util parseRefComponents, which only ever returns a ParseError. To keep it consistent for now, we should continuing to return the invalid path error, but in the future, we should consider returning ParseError where appropriate from buffetch.
| // ParseDigestType parses a DigestType from its string representation. | ||
| // | ||
| // This reverses DigestType.String(). | ||
| // | ||
| // Returns an error of type *bufparse.ParseError if the string could not be parsed. | ||
| func ParseDigestType(s string) (DigestType, error) { |
There was a problem hiding this comment.
Had an offline conversation about whether DigestType should also be a shared package (e.g. bufpkg/bufdigest), since this is also duplicated across modules and plugins. The respective keys (e.g. ModuleKey and PluginKey) types would control for the digest type at the time of construction. Our conclusion is that this makes sense, but should it be done as a follow-up PR given the size of the migration of Ref/FullName?
| @@ -75,15 +76,15 @@ func (d DigestType) String() string { | |||
| // | |||
| // This reverses DigestType.String(). | |||
| // | |||
| // Returns an error of type *ParseError if thie string could not be parsed. | |||
| // Returns an error of type *bufparse.ParseError if the string could not be parsed. | |||
| func ParseDigestType(s string) (DigestType, error) { | |||
There was a problem hiding this comment.
See comment in bufplugin/digest.go: we considered splitting digest into its own shared package. Looking at this, it does make sense, but maybe it should be done in a follow-up.
| // NewCommit returns a new Commit. | ||
| func NewCommit( | ||
| pluginKey PluginKey, | ||
| getCreateTime func() (time.Time, error), | ||
| ) Commit { | ||
| return newCommit( | ||
| pluginKey, | ||
| getCreateTime, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Similar to digest, it might make sense to split Commit into its own package as well. Same question/concern around whether that should be a follow-up PR.
This adds a new package,
bufplugin, which implements types and functions for operating on Buf plugins. It is similar to thebufmodulepackage for modules.Parsing of full names and refs has been split out into a separate package from
bufmodulecalledbufparse. This exposes two typesFullNameandRefwhich replaceModuleFullNameandModuleRefand is used by both. MethodsModuleFullName() ModuleFullNameandModuleRef() ModuleRefhave been renamed toFullName() bufparse.FullNameandRef() bufparse.Ref.The error type
ParseErrorreturned fromParse.*methods have been moved to the single typebufparse.ParseErrorupdating the packagesbufcas,bufmoduleandbufplugin.