Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Some rules do not support auto-disable #369

Open
dduugg opened this issue Mar 11, 2024 · 5 comments
Open

Some rules do not support auto-disable #369

dduugg opened this issue Mar 11, 2024 · 5 comments

Comments

@dduugg
Copy link

dduugg commented Mar 11, 2024

I'm attempting to apply the default rules set to existing proto files. My understanding is that applying -auto_disable should apply the appropriate comment for disabling. However, I still see violations for the following rules after using -auto_disable:

  - ENUM_FIELDS_HAVE_COMMENT
  - ENUMS_HAVE_COMMENT
  - FIELD_NAMES_EXCLUDE_PREPOSITIONS
  - FIELDS_HAVE_COMMENT
  - FILE_HAS_COMMENT
  - MAX_LINE_LENGTH
  - MESSAGE_NAMES_EXCLUDE_PREPOSITIONS
  - MESSAGES_HAVE_COMMENT
  - RPC_NAMES_UPPER_CAMEL_CASE
  - RPCS_HAVE_COMMENT
  - SERVICES_HAVE_COMMENT
@yoheimuta
Copy link
Owner

@dduugg Thank you for sponsoring me 😸

Let me make sure I've got your point. Regarding ENUM_FIELDS_HAVE_COMMENT, it's not mentioned in the official style guide, hence it's not enabled by default.

I was under the impression that users with files missing comments shouldn't activate ENUM_FIELDS_HAVE_COMMENT to begin with.

@dduugg
Copy link
Author

dduugg commented Mar 13, 2024

Hi @yoheimuta, happy to sponsor the project! You've saved me a lot of time.

With all_default: true, I do in fact see ENUM_FIELDS_HAVE_COMMENT violations.

What I am trying to do, is enable the default rules so that new proto files going forward are strictly enforced. However, I want inline disable the legacy violations. Excluding the files entirely is sub-optimal, because many still actively see changes.

I'm also struggling to figure out how to auto-disable custom rules. From a quick glance, it looks like RuleGen may need to add autodisable autodisable.PlacementType support, or something to that effect.

@dduugg
Copy link
Author

dduugg commented Mar 14, 2024

I took a quick stab at adding auto-disable support to custom rules (I'm new to Go, please be kind): master...dduugg:protolint:autodisable

I got stuck trying to build the proto files though, not sure what to make of this error:

$ make dev/build/proto
protoc -I _proto _proto/*.proto --go_out=plugins=grpc:internal/addon/plugin/proto
--go_out: protoc-gen-go: plugins are not supported; use 'protoc --go-grpc_out=...' to generate gRPC

See https://grpc.io/docs/languages/go/quickstart/#regenerate-grpc-code for more information.
make: *** [dev/build/proto] Error 1

@dduugg
Copy link
Author

dduugg commented Mar 18, 2024

This ticket is now low-priority, I wrote a quick wrapper to apply disables to existing violations:

#!/usr/bin/env python3

import json
import subprocess


def disable_protolint_violations() -> None:
    cmd = ["protolint", "-reporter", "json", "."]
    result = subprocess.run(cmd, stderr=subprocess.PIPE)
    # parse the json output and disable the rule at each violation:
    lints = json.loads(result.stderr)["lints"]
    if lints:
        for lint in lints:
            with open(lint["filename"], "r") as f:
                lines = f.readlines()
                line_num = lint["line"] - 1
                line = lines[line_num]
                if "protolint:disable:this" in line:
                    new_line = line[:-1] + f" {lint['rule']}\n"
                else:
                    new_line = line[:-1] + f" // protolint:disable:this {lint['rule']}\n"
                lines[line_num] = new_line
            with open(lint["filename"], "w") as f:
                f.writelines(lines)


disable_protolint_violations()
# Need to call a second time to disable line length violations
# that result from the first invocation
disable_protolint_violations()

@yoheimuta
Copy link
Owner

@dduugg Sorry for the late response.

What I am trying to do, is enable the default rules so that new proto files going forward are strictly enforced. However, I want inline disable the legacy violations. Excluding the files entirely is sub-optimal, because many still actively see changes.

I get your point. I hadn't supported the others simply because I wasn't sure if there was enough demand for it.

I took a quick stab at adding auto-disable support to custom rules (I'm new to Go, please be kind): master...dduugg:protolint:autodisable

This change looks good to me 👍

It looks like the latest protoc behave differently.
How about the following workaround for now?

20:19:04 ~/yoheimuta/protolint $ make dev/build/proto                                                                                                      git/master
# protoc -I _proto _proto/*.proto --go_out=plugins=grpc:internal/addon/plugin/proto
protoc -I _proto _proto/*.proto --go_out=. --go-grpc_out=.
rm -rf internal/addon/plugin/proto
mv github.com/yoheimuta/protolint/internal/addon/plugin/proto internal/addon/plugin/
diff --git a/Makefile b/Makefile
index 7e07d10..0cd1ee0 100644
--- a/Makefile
+++ b/Makefile
@@ -34,7 +34,11 @@ dev/install/dep:
 
 ## dev/build/proto builds proto files under the _proto directory.
 dev/build/proto:
-	protoc -I _proto _proto/*.proto --go_out=plugins=grpc:internal/addon/plugin/proto
+	# protoc -I _proto _proto/*.proto --go_out=plugins=grpc:internal/addon/plugin/proto
+	protoc -I _proto _proto/*.proto --go_out=. --go-grpc_out=.
+	rm -rf internal/addon/plugin/proto
+	mv github.com/yoheimuta/protolint/internal/addon/plugin/proto internal/addon/plugin/
 
 ## ARG is command arguments.
 ARG=lint _example/proto
diff --git a/internal/addon/plugin/proto/plugin.pb.go b/internal/addon/plugin/proto/plugin.pb.go
index cd09665..05171d6 100644
--- a/internal/addon/plugin/proto/plugin.pb.go
+++ b/internal/addon/plugin/proto/plugin.pb.go
@@ -1,7 +1,7 @@
 // Code generated by protoc-gen-go. DO NOT EDIT.
 // versions:
 //     protoc-gen-go v1.30.0
-//     protoc        v3.12.4
+//     protoc        v4.25.3
 // source: plugin.proto

 package proto
diff --git a/internal/addon/plugin/proto/plugin_grpc.pb.go b/internal/addon/plugin/proto/plugin_grpc.pb.go
index 3a6559a..21523ed 100644
--- a/internal/addon/plugin/proto/plugin_grpc.pb.go
+++ b/internal/addon/plugin/proto/plugin_grpc.pb.go
@@ -1,7 +1,7 @@
 // Code generated by protoc-gen-go-grpc. DO NOT EDIT.
 // versions:
 // - protoc-gen-go-grpc v1.3.0
-// - protoc             v3.12.4
+// - protoc             v4.25.3
 // source: plugin.proto

 package proto

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants