global options for enums and ID field names #22

Green7 · 2020-12-05T14:57:44Z

Two global options for .proto files

option (go.enum_prefix_all) = true;

When set to true, values in enum types do not have a prefix with the type name.
For example if we have in .proto:

enum TestEnum { VALUE_A = 0; }

we will get in go: VALUE_A instead of TestEnum_VALUE_A

This option works like goproto_enum_prefix_all with gogoprotobuf Extensions.

option (go.fix_id_field_names) = true

If this option is set, ID field names are converted to uppercase. So in go we have for example fields: ID, CustomerID instead of Id, CustomerId.
This option works like customnameid.go from https://github.com/containerd/containerd/tree/master/cmd/protoc-gen-gogoctrd

ydnar

Thanks! Reviewing on a phone while watching my kid, so here goes:

This should be two separate PRs for each feature.
See comments above for specific feedback.

ydnar · 2020-12-05T16:38:42Z

patch/go.proto

+
+extend google.protobuf.FileOptions {
+	optional bool enum_prefix_all = 7001;
+
+	optional bool fix_id_field_names = 7002;
+}


Stylistically, protopatch uses option messages per extended thing. It doesn't currently have a concept of a global or file-level options, but I imagine they'd be like go.file = ....

ydnar · 2020-12-05T16:40:01Z

patch/options.go

@@ -26,3 +27,19 @@ func fieldOptions(f *protogen.Field) *gopb.Options {
 func oneofOptions(o *protogen.Oneof) *gopb.Options {
 	return proto.GetExtension(o.Desc.Options(), gopb.E_Oneof).(*gopb.Options)
 }
+
+func getBoolExtension(pb proto.Message, extension protoreflect.ExtensionType, ifnotset bool) bool {


Negative assertions with Boolean fields should be prefixed or marked as such. This helper should be inverted.

ydnar · 2020-12-05T16:42:21Z

patch/options.go

+	return getBoolExtension(f.Proto.Options, gopb.E_EnumPrefixAll, false)
+}
+
+func fileFixIDFieldNamesOption(f *protogen.File) bool {


While I agree that Id should be ID, it shouldn't be special cased.

My thought for this was to have a default, possibly extensible, list of initialisms like ID, HTTP, MIME, etc. Then couple this with message or file-level options to fix them automatically.

@ydnar Thank you for the quick review.
I improved a few things:

Global options for file. Example:

option (go.file) = { disable_enum_prefixes: true, fix_initialisms: true };

I added a function to check field names and a list of commonInitialisms. This code is from golang linter.

I added the ability to set custom initialisms list:

option (go.file) = { fix_initialisms: true, initialisms: ["ID", "WHATEVER"] };

What do you think about these changes?

ydnar

Thanks for the updates—this is getting closer. A few

ydnar · 2020-12-06T15:39:00Z

patch/go.proto

+extend google.protobuf.OneofOptions { 
+	optional Options oneof = 7001; 


Trailing whitespace here. Can you run clang-format on the proto files? It should be automatic if you use VS Code and have the recommended extensions installed.

Which extension do you mean? I use VC code and vscode-proto3 extension. And it formats it differently, the above lines are replaced by:
extend google.protobuf.MessageOptions { optional Options message = 7001; }
(everything is in one line)

ydnar · 2020-12-06T18:25:02Z

patch/go.proto

+// File-level options
+message FileOptions {
+	// The disable_enum_prefixes option disables adding a prefix with a type name for an enum value.
+	optional bool disable_enum_prefixes = 1;
+
+	// The fix_initialisms option changes message field names to the golang naming convention.
+	// So for example the field named in proto file "id" would be named "ID" in go.
+	optional bool fix_initialisms = 2;
+
+	// The initialisms option contains the list of initialisms used if the fix_initialisms option is enabled.
+	// Initialisms should be in uppercase. This list may be empty, in this case the default golint initialisms
+	// list will be used.
+	repeated string initialisms = 3;
+}


I’d imagined this as a nested set of options, so one could do something like this:

option (go.initialisms) = 'ID'; option (go.initialisms) = 'URL'; option (go.values) = { prefix: '' };

I pushed a spike design here: https://github.com/alta/protopatch/blob/ydnar/file-options-design/patch/go.proto

Additionally, it would not have a default list of initialisms from golint, but explicitly work off the initialisms you specify in the file.

I’d imagined this as a nested set of options, so one could do something like this:

It is possible now. You can write:

option (go.file) = { fix_initialisms: true, initialisms: ["ID", "URL"] };

or:

option (go.file).fix_initialisms = true; option (go.file).initialisms = "ID"; option (go.file).initialisms = "URL";

Additionally, it would not have a default list of initialisms from golint, but explicitly work off the initialisms you specify in the file.

Do you want to drop the default initialism list entirely?
I think the default initialisms list is a good idea. Why? Because by enabling one option, we get a file compatible with go lint. This is hard to do by adding initialisms one by one: go lint has about 40 initialisms ...
And if you don't want to use the default list, just enter your own: then the default list is omitted. This is how the current version works.

The default initialisms list, while convenient, isn’t exhaustive. A single proto file will have a handful of these initialisms, and I think it’s better to have each file define what exactly it wants rather than depend on a predefined list of English-centric initialisms.

I think we should favor clarity over brevity.

Note in my branch I skipped go.file in favor of go.messages, go.enums, etc. to illustrate where each change would apply, to all messages, fields, enums, values, and oneofs.

The one issue with this is that having a top-level FileOptions extension initialisms means that this isn’t possible:

option (go.initialisms) = ["ID", "URL"];

I guess we could nest our own FileOptions as a single extension to FileOptions, with messages, enums, fields, values, oneofs, so this would be possible:

option (go.file).values = { prefix: '' }

The default initialisms list, while convenient, isn’t exhaustive. A single proto file will have a handful of these initialisms, and I think it’s better to have each file define what exactly it wants rather than depend on a predefined list of English-centric initialisms.

I think we should favor clarity over brevity.

Ok, but what if we have many proto files in the project? I have about 30 proto files in my small project. So in each file I have to define a list of the same initialisms. It is not very convenient to maintenance. How to solve it? Global option for all patched files?

How many initialisms does each proto file use?

In my case, not so much, probably max 3.

@ydnar Can you look on last commit ? Is it closer to your idea?

ydnar

Getting there.

ydnar · 2020-12-22T21:52:37Z

patch/patcher.go

@@ -178,7 +194,15 @@ func (p *Patcher) scanField(f *protogen.Field) {
 	if newName == "" && o != nil && (p.isRenamed(m.GoIdent) || p.isRenamed(o.GoIdent)) {
 		// Implicitly rename this oneof field because its parent(s) were renamed.
 		newName = f.GoName
+	} else {
+		if len(p.fileInitialisms) > 0 {


Thinking this should run on all names, even if renamed, so it can fix nested oneof names.

ydnar · 2020-12-22T21:53:05Z

patch/lintname.go

+)
+
+// lintName returns a different name if it should be different.
+func lintName(name string, commonInitialisms map[string]bool) (should string) {


Suggested change

func lintName(name string, commonInitialisms map[string]bool) (should string) {

func lintName(name string, initialisms map[string]bool) (should string) {

ydnar · 2020-12-22T21:53:14Z

patch/lintname.go

+	return string(runes)
+}
+
+// commonInitialisms is a set of common initialisms.


This should be removed.

ydnar · 2020-12-22T21:53:24Z

patch/patcher.go

@@ -622,3 +646,19 @@ func typeString(obj types.Object) string {
 	}
 	return obj.Type().String()
 }
+
+func newIDFieldName(fieldName string) string {


This should be removed.

ydnar · 2020-12-22T21:56:15Z

patch/patcher.go

+	fileFieldOptions  *gopb.Options
+	fileValuesOptions *gopb.Options
+	fileInitialisms   map[string]bool


These are by file, but stored here globally. One file can clobber another file’s data.

I’d prefer to see these fetched on-demand at the point of use, perhaps as a fallback if a given thing doesn’t have more specific options, it can look at the file for global options.

Please check the latest version. I have a problem with this last fix. I did fileFieldOptions,fileValuesOptions and fileInitialisms global because is hard to feetch this data at the point of use. In functions like scanEnum we don't have access to protogen.File. We would have to pass protogen.File (or fileFieldOptions etc). to the functions: scanEnum, scanMessage, scanExtension and then to scanEnumValue, scanField, etc.
Do you have a better idea?

I think it’s possible to walk up to the parent file from the protoreflect.Descriptor interface’s ParentFile method.

I’m not sure what the ideal interface (or helper interface is) to get it, but I think it should first examine the thing (say a protogen.Field) for options, and then look at its Desc.ParentFile for a global option.

ydnar · 2021-01-04T21:04:52Z

Closing in favor of #37.

enums, ID fileld names

f1f5e00

ydnar requested changes Dec 5, 2020

View reviewed changes

enums, ID fileld names

f792bea

ydnar reviewed Dec 6, 2020

View reviewed changes

enums, ID fileld names

0768ab3

ydnar reviewed Dec 22, 2020

View reviewed changes

enums, ID fileld names

39049eb

ydnar mentioned this pull request Dec 24, 2020

Support patching other Go plugins #25

Merged

enums, ID fileld names

858f406

This was referenced Dec 25, 2020

File-level go.lint option #32

Merged

File-level go.lint option #37

Merged

ydnar closed this Jan 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

global options for enums and ID field names #22

global options for enums and ID field names #22

Green7 commented Dec 5, 2020

ydnar left a comment

ydnar Dec 5, 2020

ydnar Dec 5, 2020

ydnar Dec 5, 2020

Green7 Dec 6, 2020 •

edited

Loading

ydnar left a comment

ydnar Dec 6, 2020

Green7 Dec 7, 2020

ydnar Dec 6, 2020

ydnar Dec 6, 2020

Green7 Dec 7, 2020 •

edited

Loading

ydnar Dec 7, 2020

ydnar Dec 7, 2020

Green7 Dec 7, 2020

ydnar Dec 7, 2020

Green7 Dec 7, 2020

Green7 Dec 12, 2020

ydnar left a comment

ydnar Dec 22, 2020

ydnar Dec 22, 2020

ydnar Dec 22, 2020

ydnar Dec 22, 2020

ydnar Dec 22, 2020

Green7 Dec 23, 2020

ydnar Dec 24, 2020

ydnar commented Jan 4, 2021

		extend google.protobuf.OneofOptions {
		optional Options oneof = 7001;

	func lintName(name string, commonInitialisms map[string]bool) (should string) {
	func lintName(name string, initialisms map[string]bool) (should string) {

global options for enums and ID field names #22

global options for enums and ID field names #22

Conversation

Green7 commented Dec 5, 2020

ydnar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Green7 Dec 6, 2020 • edited Loading

Choose a reason for hiding this comment

ydnar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Green7 Dec 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ydnar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ydnar commented Jan 4, 2021

Green7 Dec 6, 2020 •

edited

Loading

Green7 Dec 7, 2020 •

edited

Loading