Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: Generate enum strings for Compute #860

Merged
merged 1 commit into from
Jan 16, 2025

Conversation

jskeet
Copy link
Collaborator

@jskeet jskeet commented Jan 16, 2025

This moves code from google-cloud-dotnet to the generator, removing the post-processing.

We can probably adopt this as-is for the moment (given that this is only for DIREGAPIC, and only Compute uses DIREGAPIC) but we should really have configuration within the service config publish settings to turn this feature on (and potentially configure the filename; that'll be easier to tell if we end up with more examples).

@jskeet jskeet requested a review from a team as a code owner January 16, 2025 17:08
@jskeet
Copy link
Collaborator Author

jskeet commented Jan 16, 2025

(Note that we could remove one piece of config by generating one type per FileDescriptor, and using the name of the FileDescriptor as a prefix with a hard-coded suffix. Compute only has compute.proto. Let me know if you want me to implement that.

Testing: this is basically tested by running it against the Compute API and checking that the result is "identical" (in semantic diff terms) to the current code.

Copy link
Contributor

@amanda-tarafa amanda-tarafa left a comment

Choose a reason for hiding this comment

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

A few nits, but LGTM.

I'm totally happy with this as is, we can figure things out if we ever need to.


public static CompilationUnitSyntax MaybeGenerate(SourceFileContext ctx, string ns, IEnumerable<FileDescriptor> packageFileDescriptors)
{
// TODO: Add an option in the service config for this... although
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: seems incomplete... although I'll probably be happy with what comes after the although :)

namespace Google.Api.Generator.Generation;

/// <summary>
/// Generates (to stdout) a static class (with nested static classes) to provide
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Generates (to stdout) a static class (with nested static classes) to provide
/// Generates a static class (with nested static classes) to provide

/// <summary>
/// Generates (to stdout) a static class (with nested static classes) to provide
/// string constants for the wire representations of enum values. This is only used
/// for DIREGAPIC APIs.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// for DIREGAPIC APIs.
/// for DIREGAPIC APIs which is just the Compute API.

if (Descriptor is not null)
{
Utils.Logging.LogWarning("Typ of {name} is {fullname}", Descriptor.FullName, ProtoTyp.Of(Descriptor).FullName);
//throw new Exception($"Typ of {Descriptor.FullName} is {ProtoTyp.Of(Descriptor).FullName}");
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: leftover

This moves code from google-cloud-dotnet to the generator, removing the post-processing.

We can probably adopt this as-is for the moment (given that this is only for DIREGAPIC, and only Compute uses DIREGAPIC) but we should really have configuration within the service config publish settings to turn this feature on (and potentially configure the filename; that'll be easier to tell if we end up with more examples).
@jskeet jskeet merged commit 84d3dff into googleapis:main Jan 16, 2025
3 checks passed
@jskeet jskeet deleted the compute-enums branch January 16, 2025 17:42
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants