Skip to content
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

Merged
merged 13 commits into from
Nov 13, 2024
Merged

Add bufplugin package #3431

merged 13 commits into from
Nov 13, 2024

Conversation

emcfarlane
Copy link
Contributor

@emcfarlane emcfarlane commented Oct 29, 2024

This adds a new package, bufplugin, which implements types and functions for operating on Buf plugins. It is similar to the bufmodule package for modules.

Parsing of full names and refs has been split out into a separate package from bufmodule called bufparse. This exposes two types FullName and Ref which replace ModuleFullName and ModuleRef and is used by both. Methods ModuleFullName() ModuleFullName and ModuleRef() ModuleRef have been renamed to FullName() bufparse.FullName and Ref() bufparse.Ref.

The error type ParseError returned from Parse.* methods have been moved to the single type bufparse.ParseError updating the packages bufcas, bufmodule and bufplugin.

@emcfarlane emcfarlane requested a review from doriable October 29, 2024 20:40
Copy link
Contributor

github-actions bot commented Oct 29, 2024

The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedNov 4, 2024, 4:20 PM

@@ -0,0 +1,83 @@
// Copyright 2020-2024 Buf Technologies, Inc.
Copy link
Member

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).

Copy link
Contributor Author

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) {
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

@doriable doriable left a 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.).

private/buf/buffetch/internal/module_ref.go Show resolved Hide resolved
private/bufpkg/bufplugin/digest.go Show resolved Hide resolved
private/bufpkg/bufmodule/digest.go Show resolved Hide resolved
private/bufpkg/bufplugin/commit.go Show resolved Hide resolved
@emcfarlane emcfarlane requested a review from bufdev November 5, 2024 21:19
@emcfarlane emcfarlane merged commit 10ddcf5 into main Nov 13, 2024
10 checks passed
@emcfarlane emcfarlane deleted the ed/bufplugin branch November 13, 2024 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants