Add type option #61

Adphi · 2021-09-24T09:33:20Z

This pull request add a type field option:

        // The type option changes the generated field type.
	// All generated code assumes that this type is castable to the protocol buffer field type
	optional string type = 3;

scalar fields support

type Name string

message CastTypeMessage {
   string name = 1 [(go.field).type = "Name"]
}

repeated fields support:

type Names []string

message CastTypeMessage {
    repeated string names = 1 [(go.field).type = "Names"]
}

Adphi · 2021-09-24T10:56:03Z

@ydnar, I have allowed edits by maintainers, feel free to refactor as much as you think it needs.

ydnar

This is pretty impressive. I'm in a phone so can't test it, but I'm generally inclined to accept this PR with some changes.

Thanks!

patch/go.proto

go.mod

patch/casttype.go

patch/casttype_test.go

patch/patcher.go

tests/message/message_casttypes.proto

tests/message/message_renames.proto

ydnar · 2021-09-27T01:41:10Z

@Adphi can it support slices of imported types? e.g.:

message CastTypeMessage {
   repeated string names = 1 [(go.field).casttype = "[]github.com/user/module/pkg.Name"]
}

Adphi · 2021-09-27T09:28:06Z

@Adphi can it support slices of imported types? e.g.:

message CastTypeMessage {
   repeated string names = 1 [(go.field).casttype = "[]github.com/user/module/pkg.Name"]
}

There was an issue with the package import generation, it is fixed.
But it is not fully supported for the moment as []github.com/user/module/pkg.Name cannot be cast to []string.
The declaration patch is the easy part, the hard part is the refactoring of the generated code usage.
It would be possible to add support for this, but it's a lot more work because you would have to detect slice iterations and index access to cast the usage...

ydnar · 2021-09-27T14:37:32Z

There was an issue with the package import generation, it is fixed.

But it is not fully supported for the moment as []github.com/user/module/pkg.Name cannot be cast to []string.

The declaration patch is the easy part, the hard part is the refactoring of the generated code usage.

It would be possible to add support for this, but it's a lot more work because you would have to detect slice iterations and index access to cast the usage...

Got it.

Given that Go supports type aliases, where type A = package.A then the fully qualified version might not need to exist at all. That would simplify the API.

What do you think?

ydnar · 2021-09-27T14:42:12Z

Can you fix the go vet issues?

Adphi · 2021-09-27T14:44:22Z

Actually, It's not about type alias (type A = B), but about named type (type A string or type A []string).
The fully qualified name has nothing to do with the limitation, it is the same issue that you may have encountered when trying to cast []interface{} to []string, I will remove the []Type from the comments.

Adphi · 2021-09-27T14:46:02Z

Can you fix the go vet issues?

~~😬 I cannot reproduce the go vet issue, both on my local computer and inside a docker container...~~
Got it.

ydnar · 2021-09-27T16:03:20Z

Actually, It's not about type alias (type A = B), but about named type (type A string or type A []string).
The fully qualified name has nothing to do with the limitation, it is the same issue that you may have encountered when trying to cast []interface{} to []string, I will remove the []Type from the comments.

Sorry, I meant using type aliases for types declared in other packages.

Rather than have protopatch add additional imports, the caller can have a separate file that implements the named type in the package, e.g. instead of (go.field).type = 'github.com/org/repo.A', they can only declare package-defined types, which can be aliased in a Go file:

package myproto

import "github.com/org/repo"

type A = repo.A

This should make the implementation in protopatch significantly simpler.

Adphi · 2021-09-27T16:51:25Z

@ydnar I'm sorry, I don't understand why you would want to use type aliases. I think I must have explained myself wrong. There is no problem with external package imports: please take a look at the plugin tests' generated code.

ydnar · 2021-09-27T17:18:52Z

@ydnar I'm sorry, I don't understand why you would want to use type aliases. I think I must have explained myself wrong. There is no problem with external package imports: please take a look at the plugin tests' generated code.

Oh, I know that it works. But it adds complexity to both the plugin interface and the implementation.

Is there any functionality that requires external package imports?

tests/plugin/validate.proto

Adphi · 2021-09-27T17:51:24Z

Is there any functionality that requires external package imports?

For example: I am working on multiple active directory based projects divided into several libraries, active directory uses an attribute called userAccountControl to store security related flags. The library defines the UserAccountControl as type UserAccountControl uint32, it exposes many methods managing the different flags, both to check the presence and to set them. This library is used in multiple gRPC services.

ydnar · 2021-09-27T17:53:58Z

Is there any functionality that requires external package imports?

For example: I am working on multiple active directory based projects divided into several libraries, active directory uses an attribute called userAccountControl to store security related flags. The library defines the UserAccountControl as type UserAccountControl uint32, it exposes many methods managing the different flags, both to check the presence and to set them. This library is used in multiple gRPC services.

Got it. Can you add a single Go file that aliases UserAccountControl into the package directory?

package foo

import "github.com/Adphi/ws"

type UserAccountControl = ws.UserAccountControl

Adphi · 2021-09-27T17:57:45Z

Of course I can 😉. But I don't see any reason to do it as protopatch would allow to use the original type, and the implementation in protopatch is pretty straight forward...

ydnar · 2021-09-27T18:05:32Z

Of course I can 😉. But I don't see any reason to do it as protopatch would allow to use the original type, and the implementation in protopatch is pretty straight forward...

My concerns are three-fold:

It adds complexity to the interface. The scalar vs repeated type support is terrific, but the lack of support for repeated types from another package seems like a confusing omission. Something that I (or we) would have to document and handle inquiries about.
The language provides a mechanism to bring a type declared in an import package into a locally declared type, via aliasing. I’m not sure we need protopatch to provide duplicate functionality for something that already can be done another way.
This adds a lot of code and complexity to this package, which means more to maintain and debug over time.

I’d consider it if you could split out foreign types into a separate follow-up PR that we can evaluate in isolation. I agree that it provides some nice ergonomics to declare a fully-qualified type, but my concerns outlined above make me wary.

Adphi · 2021-09-28T12:20:01Z

@ydnar Thanks for the detailed explanation. I'll move external packages support into another pull request.

patch/field_type_test.go

ydnar · 2021-10-01T16:07:33Z

@Adphi is the description in this PR up to date with the functionality?

e.g. has fully-qualified types and []T types been removed for future PRs?

Adphi · 2021-10-01T16:44:51Z

@Adphi is the description in this PR up to date with the functionality?

It should be now 😉

ydnar

Thanks! Just a few more comments.

patch/field_type.go

ydnar

Close! Looking forward to merging this one.

patch/field_type_test.go

patch/patcher.go

ydnar · 2021-10-31T22:01:02Z

@Adphi can you rebase this against current main branch?

Adphi · 2021-10-31T23:36:51Z

@ydnar it's done.

ydnar · 2021-11-01T00:11:14Z

patch/go.proto

@@ -21,6 +21,10 @@ message Options {
 	// See https://golang.org/ref/spec#Struct_types.
 	optional bool embed = 2;

+	// The type option changes the generated field type.
+	// All generated code assumes that this type is castable to the protocol buffer field type,


Suggested change

// All generated code assumes that this type is castable to the protocol buffer field type,

// All generated code assumes that this type is castable to the protocol buffer field type.

ydnar · 2021-11-01T16:17:10Z

@Adphi merged and released in v0.5.0.

Could you make a follow-up PR that adds documentation for the type option to the README?

Adphi force-pushed the casttype branch 2 times, most recently from a16c2e5 to 416f079 Compare September 24, 2021 09:38

Adphi force-pushed the casttype branch from eadb099 to c208263 Compare September 24, 2021 10:59

Adphi marked this pull request as ready for review September 24, 2021 11:26

Adphi force-pushed the casttype branch from 5eba0c3 to 41c7ded Compare September 24, 2021 11:32

ydnar requested changes Sep 24, 2021

View reviewed changes

Adphi force-pushed the casttype branch from 9e3a642 to 9d72506 Compare September 27, 2021 09:37

Adphi changed the title ~~Add casttype support for scalar types~~ Add type option Sep 27, 2021

Adphi requested a review from ydnar September 27, 2021 10:06

Adphi force-pushed the casttype branch 2 times, most recently from 8ff3ef2 to f607bf3 Compare September 27, 2021 11:43

Adphi force-pushed the casttype branch from 17d30c4 to 5733dfd Compare September 27, 2021 15:03

ydnar reviewed Sep 27, 2021

View reviewed changes

tests/plugin/validate.proto Show resolved Hide resolved

ydnar mentioned this pull request Sep 27, 2021

GoGo Protobuf looking for new ownership gogo/protobuf#691

Closed

Adphi force-pushed the casttype branch from f3650d0 to 0f4e6a5 Compare September 28, 2021 07:12

ydnar reviewed Oct 1, 2021

View reviewed changes

patch/field_type_test.go Outdated Show resolved Hide resolved

Adphi force-pushed the casttype branch from 32cec3a to 89191df Compare October 1, 2021 16:49

ydnar requested changes Oct 1, 2021

View reviewed changes

patch/field_type.go Outdated Show resolved Hide resolved

patch/field_type.go Outdated Show resolved Hide resolved

Adphi force-pushed the casttype branch from 008e7cd to 54647cb Compare October 1, 2021 17:14

Adphi requested a review from ydnar October 1, 2021 18:39

ydnar requested changes Oct 18, 2021

View reviewed changes

patch/field_type_test.go Show resolved Hide resolved

patch/patcher.go Outdated Show resolved Hide resolved

Adphi force-pushed the casttype branch from d674338 to 3f31187 Compare October 21, 2021 10:29

Adphi requested a review from ydnar October 21, 2021 10:38

Adphi added 9 commits November 1, 2021 00:32

add field type support

2dbdd66

type: fix double cast

7d0b8d8

type: moved global test constants

9fd991a

type: remove unneeded code

a9b7ac7

type: simpler type name validation

584f99b

type: fix optional fields usage

40f577a

field types tests: fix optional getter definition

b278564

field types: fix patcher fields order

db3afcc

go fmt

2e308d5

Adphi force-pushed the casttype branch from fee79eb to 2e308d5 Compare October 31, 2021 23:35

ydnar approved these changes Nov 1, 2021

View reviewed changes

ydnar reviewed Nov 1, 2021

View reviewed changes

Adphi added 2 commits November 1, 2021 14:55

type: fix proto comment

0931de0

type: regenerate proto

399e45e

ydnar approved these changes Nov 1, 2021

View reviewed changes

ydnar merged commit 8a60d52 into alta:main Nov 1, 2021

Adphi deleted the casttype branch November 17, 2021 07:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add type option #61

Add type option #61

Adphi commented Sep 24, 2021 •

edited

Loading

Adphi commented Sep 24, 2021

ydnar left a comment

ydnar commented Sep 27, 2021

Adphi commented Sep 27, 2021 •

edited

Loading

ydnar commented Sep 27, 2021

ydnar commented Sep 27, 2021

Adphi commented Sep 27, 2021 •

edited

Loading

Adphi commented Sep 27, 2021 •

edited

Loading

ydnar commented Sep 27, 2021

Adphi commented Sep 27, 2021

ydnar commented Sep 27, 2021

Adphi commented Sep 27, 2021

ydnar commented Sep 27, 2021

Adphi commented Sep 27, 2021 •

edited by ydnar

Loading

ydnar commented Sep 27, 2021 •

edited

Loading

Adphi commented Sep 28, 2021

ydnar commented Oct 1, 2021

Adphi commented Oct 1, 2021

ydnar left a comment

ydnar left a comment

ydnar commented Oct 31, 2021

Adphi commented Oct 31, 2021

ydnar Nov 1, 2021

ydnar commented Nov 1, 2021

	// All generated code assumes that this type is castable to the protocol buffer field type,
	// All generated code assumes that this type is castable to the protocol buffer field type.

Add type option #61

Add type option #61

Conversation

Adphi commented Sep 24, 2021 • edited Loading

Adphi commented Sep 24, 2021

ydnar left a comment

Choose a reason for hiding this comment

ydnar commented Sep 27, 2021

Adphi commented Sep 27, 2021 • edited Loading

ydnar commented Sep 27, 2021

ydnar commented Sep 27, 2021

Adphi commented Sep 27, 2021 • edited Loading

Adphi commented Sep 27, 2021 • edited Loading

ydnar commented Sep 27, 2021

Adphi commented Sep 27, 2021

ydnar commented Sep 27, 2021

Adphi commented Sep 27, 2021

ydnar commented Sep 27, 2021

Adphi commented Sep 27, 2021 • edited by ydnar Loading

ydnar commented Sep 27, 2021 • edited Loading

Adphi commented Sep 28, 2021

ydnar commented Oct 1, 2021

Adphi commented Oct 1, 2021

ydnar left a comment

Choose a reason for hiding this comment

ydnar left a comment

Choose a reason for hiding this comment

ydnar commented Oct 31, 2021

Adphi commented Oct 31, 2021

ydnar Nov 1, 2021

Choose a reason for hiding this comment

ydnar commented Nov 1, 2021

Adphi commented Sep 24, 2021 •

edited

Loading

Adphi commented Sep 27, 2021 •

edited

Loading

Adphi commented Sep 27, 2021 •

edited

Loading

Adphi commented Sep 27, 2021 •

edited

Loading

Adphi commented Sep 27, 2021 •

edited by ydnar

Loading

ydnar commented Sep 27, 2021 •

edited

Loading