cretz
commented
2 years ago You don't need to do anything to this repository to get the protos the way you want for your SDK.
I have had many cases where I move generated classes around, rename them, etc harmlessly. This does not affect the source protos nor should it. Make your generated protos look however you want to look in your SDK. In fact, you probably should as the default protoc for most languages is ugly. If that means as part of your build script you take the protos and move them around the way your draft PR does, no problem. If that means advanced post processing, no problem. That doesn't need to affect this repository.
We do some advanced proto generation post-processing in the Python SDK for example because the generated result is not to our liking.
Having said that, I do support changing the csharp_namespace if we really must. But the current values seem reasonable to me for consistency (in our other SDK's, we call it "api" not "service api" and we have the version as the last member of the package...would need a good reason to make C# special).
macrogreg
Hi folks,
I have a proposal (incl. a sample solution) that I would like to discuss. If folks find the following interesting in general, I have a draft PR, for which I'd love to collect feedback.
Here, I am making a critical assumption on which the need to this proposal is based. Let us assume that the assumption is, at least partially, correct. If there are strong concerns with it, we can get back to it later:
Here, I am proposing an improvement of that kind. 😃
Context
Our proto files specify, among other things, what package names (aka namespaces) the generated language APIs should have in various target languages. So far we have left them unchanged, simply following the directory structure in the API repo. For established languages this is now a done deal, as changing the package names is a compatibility break. For new / not-established languages, we may have some more flexibility to improve things (if needed).
Problem
While designing and developing the .NET SDK I have met the need to constantly import a multitude of
Temporal.Api.Xyznamespaces. It feels quite redundant and annoying. Doing it once is not a big deal. But in practice, you have to do it for EVERY SINGLE FILE that uses Temporal's data contracts in your application. And in a non-trivial application, that can be quite many files. Java users may not fully appreciate this, as they can simply importTemporal.Api.*, but such a wildcard syntax does not exist in all languages.Of course, the IDE can help doing this quickly. But just like with the
*-syntax, the convenience of it varies across tools and environments. Some developers simply prefer using notepad-like editors. In other cases, the advanced IDEs' convenience factors for this particular scenario vary too. For example, Visual Studio does not automatically import namespaces for good reasons that go beyond the scope of this discussion. Instead, you have to click on a type that is not yet "found" (or hit a hot-key while the cursor is there) and then select an item in the appearing drop-down menu, and VS will then import the namespace. You have to do it every time a type is not found. In our case - once per required Temporal namespace in each file that needs that namespace. So basically - a lot.Solution
One of our guiding principles is focus on our users/developer. And we can make their lives better by improving on the package/namespace structure of temporal APIs. Crucially, we can only do it for new or almost new Temporal languages.
For example, for .NET, I would consider it a great story to reduce all the namespaces to the following four:
Explanation:
Root namespace:
Temporal.ServiceApi.V1....Temporal.ServiceApi.V1.WorkflowServiceThe RPC declarations (and only the RPC declarations) for the workflow service. No data exchange types. Essentially, what is currently declared in workflowservice/v1/service.proto.
Temporal.ServiceApi.V1.OperatorServiceThe RPC declarations (and only the RPC declarations) for the operator service. No data exchange types. Essentially, what is currently declared in operatorservice/v1/service.proto.
Temporal.ServiceApi.V1.DataModelAll data exchange types. There is no reason for distinct namespaces. For the user, this is the data model for interacting with the Temporal service. Currently, this includes everything that is not in the other namespaces.
Temporal.ServiceApi.V1.DataModel.ErrorDetailsThe contracts here are special because they are not really part of the API. In fact, to my knowledge, currently only Go, Java, Python, and C++ actually support the gRPC richer error model on which these are based.
Engineering considerations
I prototyped making the proposed improvement for .NET while leaving other languages unchanged. Initially, it required to changing the namespace setting in each file accordingly. E.g.:
to
Unfortunately, that is not enough. The thing is that some proto compilers use the file name as a container name when grouping generated code. If the same file name exists in different namespaces, it is not a problem. However, the same file name cannot exist in the same namespace/package more then once, even if it is placed in different directories. Yet, call almost everything
message.proto. I addressed that with a structural renaming. E.g.:temporal/api/common/v1/message.proto-->temporal/api/common/v1/common_message.prototemporal/api/failure/v1/message.proto-->temporal/api/failure/v1/failure_message.prototemporal/api/taskqueue/v1/message.proto-->temporal/api/taskqueue/v1/taskqueue_message.proto. . .This did the trick and the protos compiled just fine. Only the .NET generated files should be different from before. Here is a draft PR: https://github.com/temporalio/api/pull/170.
Alternative option:
Note that once we are already forced to do a file renaming like above, we are missing an opportunity to remove some redundant characters in the file names. For example:
temporal/api/common/v1/message.proto-->temporal/api/v1/common_message.prototemporal/api/failure/v1/message.proto-->temporal/api/v1/failure_message.prototemporal/api/taskqueue/v1/message.proto-->temporal/api/v1/taskqueue_message.proto. . .As a result of that, all the files end up in the same directory. Whether or not it is a good thing is a matter of taste, and various constraints, some of which are discussed below. Either way, here is a draft PR: https://github.com/temporalio/api/pull/171.
Summary:
I validated that both of the above options compile fine to .NET, using the proposed namespace structure. Other languages should be unchanged. However, the file renaming offers other (new) languages the option to do their own namespace mappings if they wish to.
If people are interested in this proposal, I'd be more than happy to validate all other relevant languages to remain unaffected by this restructuring of source file locations.
Please, let me know what you think.