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

feat(generator): support loading proto mixins #256

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

codyoss
Copy link
Member

@codyoss codyoss commented Nov 21, 2024

No description provided.

return err
}
// Set the source package. We should always take the first service registered
// as the source package. Services with mixes well register those after the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/mixes well/mixes will/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works well for mixins, but I am not sure is great to safely handle any source directory. Recall that protobuf inputs are specified as directories, and all the files in the directory are provided to the parser:

func determineInputFiles(config genclient.ParserOptions) ([]string, error) {

With this change, if I accidentally provide google/cloud/ as the input sidekick will try to generate something. I am not sure that is what we want.

api.Messages = append(api.Messages, msg)
} else {
slog.Warn("missing message in symbol table", "message", mFQN)
if !isMixinPackage(f.GetPackage()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have said

  if isMixinPackage(f.GetPackage()) {
      continue;
  }

"-codec-option", "package:gtype=package=type-golden-gclient,path=generator/testdata/rust/gclient/golden/type,source=google.type",
},
},
// {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a remnant of a local testing trick?

@@ -26,6 +26,8 @@ use gax::error::{Error, HttpError};
use google_cloud_auth::{Credential, CredentialConfig};
use std::sync::Arc;

const DEFAULT_HOST: &str = "https://{{DefaultHost}}/";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change seems unrelated. Nice change, but maybe should have its own PR. Ditto for the client.go.mustache change.

Comment on lines +35 to +39
var enabledMixins = map[string]bool{
locationPackage: false,
iamPackage: false,
longrunningService: false,
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a property of the API.

Eventually we may want to run multiple Codec / parsers in the same sidekick instance (e.g. to regenerate the world using multiple goroutines).

Copy link
Contributor

@coryan coryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should talk about some of these changes. Though maybe you intended to just test this PR and not have it reviewed yet.

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.

2 participants