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

suggestion: Use constant names rather than literals #59

Open
seebs opened this issue Nov 23, 2019 · 5 comments
Open

suggestion: Use constant names rather than literals #59

seebs opened this issue Nov 23, 2019 · 5 comments

Comments

@seebs
Copy link

seebs commented Nov 23, 2019

When using enumer, it's possible to end up with something where the only places that a constant would be directly used would be in the conversion to/from string, but enumer doesn't use the constant, it uses a literal value.

See examples here:
dominikh/go-tools#660

So for instance

var _DayNameToValueMap = map[string]Day{
	_DayName[0:6]:   0,
	_DayName[6:13]:  1,
	_DayName[13:22]: 2,
	_DayName[22:30]: 3,
	_DayName[30:36]: 4,
	_DayName[36:44]: 5,
	_DayName[44:50]: 6,
}

This could use Monday instead of the literal 0, and then it would be easier for analysis tools to detect that the constant value is in some way used. It might also be easier to read the resulting code, although I recognize that's not especially important with generated code.

@domsekotill
Copy link

How about a compile-time check in the form of a non-executable function like stringer? It has the benefit of causing an "invalid array index" error during compilation if go generate is not run after the constants are reordered.

An example with day names:

func _() {
	// An "invalid array index" compiler error signifies that the constant values have changed.
	// Re-run the stringer command to generate them again.
	var x [1]struct{}
	_ = x[Monday-0]
	_ = x[Tuesday-1]
	_ = x[Wednesday-2]
	_ = x[Thursday-3]
	_ = x[Friday-4]
	_ = x[Saturday-5]
	_ = x[Sunday-6]
}

@andig
Copy link

andig commented Apr 27, 2020

@dmarkham do you want to open a PR here?

@dmarkham
Copy link

@andig unfortunately my fork is no longer compatible, mainly because I have pulled in some of the PRs and ideas from here and went a slightly different direction. My PR is referenced here mainly to let people know I liked and used the idea, as a thank you.

@andig
Copy link

andig commented Apr 27, 2020

@dmarkham @alvaroloes is there any overarching plan if both packages serve a different purpose in the future or should both continue to be maintained?

@dmarkham
Copy link

dmarkham commented Apr 27, 2020

@dmarkham @alvaroloes is there any overarching plan if both packages serve a different purpose in the future or should both continue to be maintained?

Well in my mind these packages are just band-aids, waiting for real Enums in go. This package is widely used and stable. My package has more bells and whistles, that I wanted after researching what other forks, issues, and PRs that were getting submitted. They both are important. I just really wanted some of these features myself, and this package is just more conservative thus stable, and I think that's a good thing. I would rather move quickly, add the features, and I think that's a good thing also.

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

4 participants