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

proposal: go/printer: don't reformat single line switch cases #57667

Closed
i4ki opened this issue Jan 7, 2023 · 9 comments
Closed

proposal: go/printer: don't reformat single line switch cases #57667

i4ki opened this issue Jan 7, 2023 · 9 comments
Labels
Milestone

Comments

@i4ki
Copy link

i4ki commented Jan 7, 2023

The snippet below:

func (tt Type) String() string {
	switch tt {
	case LPAREN: 	return "("
	case LBRACK: 	return "["
	case LBRACE: 	return "{"
	case RPAREN: 	return ")"
	case RBRACK: 	return "]"
	case RBRACE: 	return "}"
	case ASSIGN: 	return "="
	case ADD: 	return "+"
	case SUB: 	return "-"
	case MUL: 	return "*"
	case QUO: 	return "/"
	case REM: 	return "%"
	case EQL: 	return "=="
	case GTR: 	return ">"
	case LSS: 	return "<"
	case NEQ: 	return "!="
	case GEQ: 	return ">="
	case LEQ: 	return "<="
	case DOT: 	return "."
	case COMMA:	return ","
	case COLON: 	return ":"
	case IDENT:	return "IDENT"
	case STR: 	return "STRING"
	case NUM: 	return "NUMBER"
	}
	return "UNKNOWN"
}

looks like easier to read and maintain than the one below (from gofmt):

func (tt Type) String() string {
	switch tt {
	case LPAREN:
		return "("
	case LBRACK:
		return "["
	case LBRACE:
		return "{"
	case RPAREN:
		return ")"
	case RBRACK:
		return "]"
	case RBRACE:
		return "}"
	case ASSIGN:
		return "="
	case ADD:
		return "+"
	case SUB:
		return "-"
	case MUL:
		return "*"
	case QUO:
		return "/"
	case REM:
		return "%"
	case EQL:
		return "=="
	case GTR:
		return ">"
	case LSS:
		return "<"
	case NEQ:
		return "!="
	case GEQ:
		return ">="
	case LEQ:
		return "<="
	case DOT:
		return "."
	case COMMA:
		return ","
	case COLON:
		return ":"
	case IDENT:
		return "IDENT"
	case STR:
		return "STRING"
	case NUM:
		return "NUMBER"
	}
	return "UNKNOWN"
}

This proposal is similar to #57645 but applied to switch with single-line cases.
I don't know if it's related to my dyslexia but I had a piece of code printed (on paper) with big switch cases like this and it's difficult for the eyes to match each case with its return line.

I know it's a very subjective topic... :(

@shasderias
Copy link

For consideration, how the snippet looks like when folded in IntelliJ.

image

@beoran
Copy link

beoran commented Jan 7, 2023

I think, just like for if statement, the case statement with the single lines is actually easier to read as long as the return is simple.

@greatroar
Copy link

greatroar commented Jan 7, 2023

Assuming that Type is an integer type and its values come from a simple iota, you don't need a switch:

var ttStr = [...]string{
	LPAREN: "(",
	LBRACK: "[",
	LBRACE: "{",
	RPAREN: ")",
	RBRACK: "]",
	RBRACE: "}",
	ASSIGN: "=",
	ADD:    "+",
	SUB:    "-",
	MUL:    "*",
	QUO:    "/",
	REM:    "%",
	EQL:    "==",
	GTR:    ">",
	LSS:    "<",
	NEQ:    "!=",
	GEQ:    ">=",
	LEQ:    "<=",
	DOT:    ".",
	COMMA:  ",",
	COLON:  ":",
	IDENT:  "IDENT",
	STR:    "STRING",
	NUM:    "NUMBER",
}

func (tt Type) String() string {
	if tt < 0 || int(tt) >= len(ttStr) {
		return "UNKNOWN"
	}
	return ttStr[tt]
}

@ianlancetaylor
Copy link
Contributor

I'm opposed to this. The advantage of gofmt is that it provides a single formatting guideline and thus eliminates a lot of style discussions. It's nobody favorite format, but it's OK.

I think that #57645 is at least worth discussing because there is so much discussion about error handling in Go. I can't recall any discussion about the formatting of switch statements. That is, I think that #57645 is an exception, and we shouldn't use that as a basis for providing more formatting options.

@i4ki
Copy link
Author

i4ki commented Jan 9, 2023

I understand but just to clarify, I found the #57645 just when I came here to create the proposal. I don't even agree with the linked issue, just pointed out it's a similar proposal.
I love gofmt and I'm happy with the style 99.9% of the time.

@ianlancetaylor feel free to close this.

@apparentlymart
Copy link

apparentlymart commented Jan 11, 2023

I do agree that the one-line form shown in the opening comment is more readable to me than what gofmt reformats it to.

However, I do find myself wondering where the "breaking point" is that would make it no longer more readable to format it that way. I assume that there's some level of complexity of return operand at which the line becomes too long or the expression too nested for this "tabular" presentation to remain readable.

I don't feel strongly about this proposal and would not be upset if it were rejected but if it is accepted then I think we'd need to decide a rule for what expressions are considered simple enough to be more readable in the tabular shape.

My sense of it would be that it's only allowed if the return operand is one of the following:

  • A single quoted string literal, or a raw string literal which doesn't include any newlines. (One could consider imposing a length limit on the string, but I wouldn't.)
  • A single numeric, boolean, or rune constant.
  • A single qualified identifier or unqualified identifier.

With that said, despite the slight increase in readability in this proposal, I do prefer the simplicity of the current rule that just treats all case statements as equivalent regardless of what follows them, and so I voted 👎 .

@arp242
Copy link

arp242 commented Jan 22, 2023

There is some prior art for this in how gofmt allows single-line functions if they're short enough:

func a() int   { return 1 }
func two() int { return 2 }

And even:

func a() int   { x := 1; return x }
func two() int { x := 2; return x }

If it gets too long (~120 columns IIRC) it will break to the next line.

This is useful because sometimes you have a bunch of very simple functions, and writing it on one line is fine.

I've written code similar to the OP a few times, and found myself wishing gofmt would allow single-line case for simple short statements, similar to what it allows for functions.

@rsc
Copy link
Contributor

rsc commented Jun 7, 2023

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Jun 14, 2023

No change in consensus, so declined.
— rsc for the proposal review group

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

No branches or pull requests

9 participants