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

Protolint substituting "message" with " sage" #139

Closed
navaneeth-spotnana opened this issue Dec 4, 2020 · 10 comments
Closed

Protolint substituting "message" with " sage" #139

navaneeth-spotnana opened this issue Dec 4, 2020 · 10 comments
Assignees
Labels
bug Something isn't working

Comments

@navaneeth-spotnana
Copy link

I am running protolint version 0.26.1(84c57fd) running on macOS 11.0.1, and it's making this wrong transformation:

image

Looks like it's making a wrong transformation. To reporoduce the issue, save the below text into a .proto file:

syntax = "proto3";

package foobar;

option java_package = "com.test.foo.bar";

message TestMessage { string test_field = 1; }

Run:

protolint lint -v -fix test.proto

It prints the below output to console:

2020/12/04 21:05:04 [DEBUG] Text=[syntax], Token=[2], Pos=[test.proto:1:1] called from lexer.go:139
2020/12/04 21:05:04 [DEBUG] Text=[syntax], Token=[22], Pos=[test.proto:1:1] called from lexer.go:139
2020/12/04 21:05:04 [DEBUG] Text=[=], Token=[10], Pos=[test.proto:1:8] called from syntax.go:63
2020/12/04 21:05:04 [DEBUG] Text=["], Token=[11], Pos=[test.proto:1:10] called from syntax.go:68
2020/12/04 21:05:04 [DEBUG] Text=[proto3], Token=[2], Pos=[test.proto:1:11] called from syntax.go:73
2020/12/04 21:05:04 [DEBUG] Text=["], Token=[11], Pos=[test.proto:1:17] called from syntax.go:79
2020/12/04 21:05:04 [DEBUG] Text=[;], Token=[8], Pos=[test.proto:1:18] called from syntax.go:84
2020/12/04 21:05:04 [DEBUG] Text=[package], Token=[2], Pos=[test.proto:3:1] called from lexer.go:139
2020/12/04 21:05:04 [DEBUG] Text=[package], Token=[2], Pos=[test.proto:3:1] called from lexer.go:139
2020/12/04 21:05:04 [DEBUG] Text=[package], Token=[2], Pos=[test.proto:3:1] called from parser.go:44
2020/12/04 21:05:04 [DEBUG] Text=[package], Token=[29], Pos=[test.proto:3:1] called from lexer.go:139
2020/12/04 21:05:04 [DEBUG] Text=[package], Token=[29], Pos=[test.proto:3:1] called from lexer.go:139
2020/12/04 21:05:04 [DEBUG] Text=[foobar], Token=[2], Pos=[test.proto:3:9] called from fullIdent.go:8
2020/12/04 21:05:04 [DEBUG] Text=[;], Token=[8], Pos=[test.proto:3:15] called from fullIdent.go:15
2020/12/04 21:05:04 [DEBUG] Text=[;], Token=[8], Pos=[test.proto:3:15] called from package.go:55
2020/12/04 21:05:04 [DEBUG] Text=[option], Token=[2], Pos=[test.proto:5:1] called from lexer.go:139
2020/12/04 21:05:04 [DEBUG] Text=[option], Token=[2], Pos=[test.proto:5:1] called from lexer.go:139
2020/12/04 21:05:04 [DEBUG] Text=[option], Token=[2], Pos=[test.proto:5:1] called from parser.go:44
2020/12/04 21:05:04 [DEBUG] Text=[option], Token=[30], Pos=[test.proto:5:1] called from lexer.go:139
2020/12/04 21:05:04 [DEBUG] Text=[option], Token=[30], Pos=[test.proto:5:1] called from lexer.go:139
2020/12/04 21:05:04 [DEBUG] Text=[java_package], Token=[2], Pos=[test.proto:5:8] called from option.go:153
2020/12/04 21:05:04 [DEBUG] Text=[=], Token=[10], Pos=[test.proto:5:21] called from option.go:175
2020/12/04 21:05:04 [DEBUG] Text=[=], Token=[10], Pos=[test.proto:5:21] called from option.go:58
2020/12/04 21:05:04 [DEBUG] Text=["], Token=[11], Pos=[test.proto:5:23] called from lexer.go:154
2020/12/04 21:05:04 [DEBUG] Text=["com.test.foo.bar"], Token=[6], Pos=[test.proto:5:23] called from lexer.go:139
2020/12/04 21:05:04 [DEBUG] Text=[;], Token=[8], Pos=[test.proto:5:41] called from lexer.go:139
2020/12/04 21:05:04 [DEBUG] Text=[;], Token=[8], Pos=[test.proto:5:41] called from option.go:68
2020/12/04 21:05:04 [DEBUG] Text=[message], Token=[2], Pos=[test.proto:7:1] called from lexer.go:139
2020/12/04 21:05:04 [DEBUG] Text=[message], Token=[2], Pos=[test.proto:7:1] called from lexer.go:139
2020/12/04 21:05:04 [DEBUG] Text=[message], Token=[2], Pos=[test.proto:7:1] called from parser.go:44
2020/12/04 21:05:04 [DEBUG] Text=[message], Token=[26], Pos=[test.proto:7:1] called from lexer.go:139
2020/12/04 21:05:04 [DEBUG] Text=[message], Token=[26], Pos=[test.proto:7:1] called from lexer.go:139
2020/12/04 21:05:04 [DEBUG] Text=[TestMessage], Token=[2], Pos=[test.proto:7:9] called from message.go:76
2020/12/04 21:05:04 [DEBUG] Text=[{], Token=[14], Pos=[test.proto:7:21] called from message.go:106
2020/12/04 21:05:04 [DEBUG] Text=[string], Token=[2], Pos=[test.proto:7:23] called from lexer.go:139
2020/12/04 21:05:04 [DEBUG] Text=[string], Token=[2], Pos=[test.proto:7:23] called from message.go:114
2020/12/04 21:05:04 [DEBUG] Text=[string], Token=[2], Pos=[test.proto:7:23] called from lexer.go:139
2020/12/04 21:05:04 [DEBUG] Text=[string], Token=[2], Pos=[test.proto:7:23] called from lexer.go:139
2020/12/04 21:05:04 [DEBUG] Text=[string], Token=[2], Pos=[test.proto:7:23] called from lexer.go:139
2020/12/04 21:05:04 [DEBUG] Text=[string], Token=[2], Pos=[test.proto:7:23] called from lexer.go:139
2020/12/04 21:05:04 [DEBUG] Text=[string], Token=[2], Pos=[test.proto:7:23] called from lexer.go:139
2020/12/04 21:05:04 [DEBUG] Text=[string], Token=[2], Pos=[test.proto:7:23] called from field.go:210
2020/12/04 21:05:04 [DEBUG] Text=[test_field], Token=[2], Pos=[test.proto:7:30] called from field.go:79
2020/12/04 21:05:04 [DEBUG] Text=[=], Token=[10], Pos=[test.proto:7:41] called from field.go:85
2020/12/04 21:05:04 [DEBUG] Text=[1], Token=[3], Pos=[test.proto:7:43] called from lexer.go:139
2020/12/04 21:05:04 [DEBUG] Text=[;], Token=[8], Pos=[test.proto:7:44] called from field.go:119
2020/12/04 21:05:04 [DEBUG] Text=[;], Token=[8], Pos=[test.proto:7:44] called from field.go:100
2020/12/04 21:05:04 [DEBUG] Text=[}], Token=[15], Pos=[test.proto:7:46] called from lexer.go:139
2020/12/04 21:05:04 [DEBUG] Text=[}], Token=[15], Pos=[test.proto:7:46] called from lexer.go:139
2020/12/04 21:05:04 [DEBUG] Text=[}], Token=[15], Pos=[test.proto:7:46] called from lexer.go:139
2020/12/04 21:05:04 [DEBUG] Text=[}], Token=[15], Pos=[test.proto:7:46] called from message.go:151
2020/12/04 21:05:04 [DEBUG] Text=[], Token=[1], Pos=[test.proto:7:47] called from lexer.go:189
2020/12/04 21:05:04 [DEBUG] Text=[], Token=[1], Pos=[test.proto:7:47] called from lexer.go:139
2020/12/04 21:05:04 [DEBUG] Text=[], Token=[1], Pos=[test.proto:7:47] called from lexer.go:139
2020/12/04 21:05:04 [DEBUG] Text=[], Token=[1], Pos=[test.proto:7:47] called from parser.go:44
[test.proto:7:23] Found an incorrect indentation style "   ". "  " is correct.
@yoheimuta yoheimuta added the bug Something isn't working label Dec 5, 2020
@yoheimuta
Copy link
Owner

@navaneeth-spotnana Thank you for reporting a bug.
The current implementation seems to have the assumption that a field of the message is at the next line.
This should be fixed.

@navaneeth-spotnana
Copy link
Author

I observed similar behaviour with return statements:

image

syntax = "proto3";

package foobar;

option java_package = "com.test.foo.bar";

message TestMessage { string test_field = 1; }

service FooBarService {
  rpc Put(TestMessage)
    returns (TestMessage) {}
}

Are you going to take this up and fix it? Golang is not very familiar for me, I can take a stab at it otherwise.

@navaneeth-spotnana
Copy link
Author

I was reading the code, and this is what I understand:

func (v indentVisitor) validateIndent(
	pos meta.Position,
) {
	line := v.protoLines[pos.Line-1]
	leading := ""
	for _, r := range string([]rune(line)[0 : pos.Column-1]) {
		if unicode.IsSpace(r) {
			leading += string(r)
		}
	}

	indentation := strings.Repeat(v.style, v.currentLevel)
	if leading == indentation {
		return
	}
	v.AddFailuref(
		pos,
		`Found an incorrect indentation style "%s". "%s" is correct.`,
		leading,
		indentation,
	)

	if v.fixMode {
		v.indentFixes[pos.Line-1] = indentFix{
			currentChars: len(leading),
			replacement:  indentation,
		}
	}
}

The above code snippet won't work in case there are more than one elements on the same line. What would be the fix for this?

@yoheimuta
Copy link
Owner

@navaneeth-spotnana Thank you for investigating this issue!
That's right. First, I think it's necessary to decide the solution users can satisfy.

What do you think about the change that protolint will do nothing against inner elements on the same line?

PS. If someone wants to format a file like converting to multiple lines with correct indentation, it would be better to support a new format command which rewrites the file completely according to the opinionated rule.

@navaneeth-spotnana
Copy link
Author

navaneeth-spotnana commented Dec 11, 2020

What do you think about the change that protolint will do nothing against inner elements on the same line?

👎🏽 Please don't. That's very important for me. I am considering using protolint inside our company, and if it doesn't insert proper linebreaks, it will be disappointing.

PS. If someone wants to format a file like converting to multiple lines with correct indentation, it would be better to support a new format command which rewrites the file completely according to the opinionated rule.

Agreed, I had the same thoughts while going through the code last week. Full rewrite might be easier than editing.

@yoheimuta yoheimuta self-assigned this Dec 13, 2020
@yoheimuta
Copy link
Owner

@navaneeth-spotnana Thank you for sharing your thought.
I'm working on this issue at the moment. It's almost done, so wait a while.

@daltonfury42
Copy link

In a way, full rewrite might be better, as this might enable us to support file structure guidelines?

@yoheimuta
Copy link
Owner

@daltonfury42 That's right.
I created an issue #142 for the moment.

@yoheimuta
Copy link
Owner

@navaneeth-spotnana I fixed this issue by #141.
It also features inserting proper linebreaks by default.

Can you confirm v0.27.0 works for you?

@navaneeth-spotnana
Copy link
Author

Yes it's working, thanks a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants