-
Notifications
You must be signed in to change notification settings - Fork 285
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assume this would be pulled out to a ParseFullName
for example
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.).
This adds a new package,
bufplugin
, which implements types and functions for operating on Buf plugins. It is similar to thebufmodule
package for modules.Parsing of full names and refs has been split out into a separate package from
bufmodule
calledbufparse
. This exposes two typesFullName
andRef
which replaceModuleFullName
andModuleRef
and is used by both. MethodsModuleFullName() ModuleFullName
andModuleRef() ModuleRef
have been renamed toFullName() bufparse.FullName
andRef() bufparse.Ref
.The error type
ParseError
returned fromParse.*
methods have been moved to the single typebufparse.ParseError
updating the packagesbufcas
,bufmodule
andbufplugin
.