Skip to content

Conversation

@nipunn1313
Copy link
Contributor

Mypy-protobuf is here
https://github.com/nipunn1313/mypy-protobuf

It currently uses extensions as explained here.
https://github.com/nipunn1313/mypy-protobuf/blob/main/proto/mypy_protobuf/extensions.proto

It's a fairly stable project for generating type stubs for protobuf.
It's been around since ~2015 (open source since 2017). Since open
sourcing it, it would make sense to reserve some extension numbers in
the global range.

See
nipunn1313/mypy-protobuf#396

Mypy-protobuf is here
https://github.com/nipunn1313/mypy-protobuf

It currently uses extensions as explained here.
https://github.com/nipunn1313/mypy-protobuf/blob/main/proto/mypy_protobuf/extensions.proto

It's a fairly stable project for generating type stubs for protobuf.
It's been around since ~2015 (open source since 2017). Since open
sourcing it, it would make sense to reserve some extension numbers in
the global range.

See
nipunn1313/mypy-protobuf#396
@fowles
Copy link
Contributor

fowles commented Aug 23, 2022

It seems like you only need one extension number since you are using a message with subfields.

@nipunn1313
Copy link
Contributor Author

nipunn1313 commented Aug 24, 2022

Hi @fowles - appreciate the quick turnaround.

The proto extension (as used today) uses three option extensions (from https://github.com/nipunn1313/mypy-protobuf/blob/main/proto/mypy_protobuf/extensions.proto)

extend google.protobuf.FieldOptions {
    // Tells mypy to use a specific newtype rather than the normal type for this field.
    optional string casttype = 60000;

    // Tells mypy to use a specific type for keys; only makes sense on map fields
    optional string keytype = 60002;
    // Tells mypy to use a specific type for values; only makes sense on map fields
    optional string valuetype = 60003;
}

Example usage is like this (from https://github.com/nipunn1313/mypy-protobuf/blob/main/proto/testproto/test.proto#L70):

message Simple1 {
  optional uint32 user_id = 21 [(mypy_protobuf.casttype)="test/test_generated_mypy.UserId"];
  optional string email = 22 [(mypy_protobuf.casttype)="test/test_generated_mypy.Email"];
  map<uint32, string> email_by_uid = 23 [
    (mypy_protobuf.keytype)="test/test_generated_mypy.UserId",
    (mypy_protobuf.valuetype)="test/test_generated_mypy.Email"
  ];
}

Which generates .pyi files like this (from https://github.com/nipunn1313/mypy-protobuf/blob/main/test/generated/testproto/test_pb2.pyi#L165):

class Simple1:
    user_id: test.test_generated_mypy.UserId
    email: test.test_generated_mypy.Email
    @property
    def email_by_uid(self) -> google.protobuf.internal.containers.ScalarMap[test.test_generated_mypy.UserId, test.test_generated_mypy.Email]: ...

As you can see here, there are 3 options used (so far).

I spent some time reading through the docs and found this - https://developers.google.com/protocol-buffers/docs/proto#customoptions - specifically Usually you only need one extension number. You can declare multiple options with only one extension number by putting them in a sub-message:.

We could do this, however it would be a non-backward compatible change to mypy-protobuf.
We'd have to convert to

message Options {
    // Tells mypy to use a specific newtype rather than the normal type for this field.
    optional string casttype = 1;
    // Tells mypy to use a specific type for keys; only makes sense on map fields
    optional string keytype = 2;
    // Tells mypy to use a specific type for values; only makes sense on map fields
    optional string valuetype = 3;
}

extend google.protobuf.FieldOptions {
    optional Options options = 1151;
}

Callsite would have to switch to:

message Simple1 {
  optional uint32 user_id = 21 [(mypy_protobuf.options).casttype="test/test_generated_mypy.UserId"];
  optional string email = 22 [(mypy_protobuf.options).casttype="test/test_generated_mypy.Email"];
  map<uint32, string> email_by_uid = 23 [
    (mypy_protobuf.options).keytype="test/test_generated_mypy.UserId",
    (mypy_protobuf.options).valuetype="test/test_generated_mypy.Email"
  ];
}

I would really prefer to be able to make this switch in a code-compatible way, by reserving at least 3 numbers.

One possibility is that we could reserve 4 numbers. 3 for the legacy options and a 4th one for the Options struct approach outlined above. This has the benefit of being backward compatible, but also means that going forward, we'll only be using a single Options struct extension number, even if we add new option functionality in the future.

@nipunn1313
Copy link
Contributor Author

I went ahead and implemented this "reserve 4 numbers" strategy.
It's now visible here https://github.com/nipunn1313/mypy-protobuf/blob/main/proto/mypy_protobuf/extensions.proto

3 legacy numbers (for backward compat) and one main number going forward.

@fowles fowles merged commit fb5bd9a into protocolbuffers:main Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants