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

internal/codegen: add Enum.Valid and AllEnumValues #1613

Merged
merged 2 commits into from
May 23, 2022

Conversation

josharian
Copy link
Contributor

This commit adds a Valid method for enum types
and an All...Values function to get all enum values.

This makes it easier to work with enums.

Fixes #1607

Copy link
Collaborator

@kyleconroy kyleconroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making a first pass at this. A few things before this is ready to merge.

For enums with many values, the generated code takes up a large amount of horizontal space. Can we put each enum on its own line for both methods?

func (e BooksBookType) Valid() bool {
	switch e {
	case BooksBookTypeFICTION:
		return true
	case BooksBookTypeNONFICTION:
		return true
	}
	return false
}

func AllBooksBookTypeValues() []BooksBookType {
	return []BooksBookType{
		BooksBookTypeFICTION,
		BooksBookTypeNONFICTION,
	}
}

This feature has only been requested by a few people. I'd like to put both behind a feature flag. Let's go with emit_enum_valid_method and emit_all_enum_values. I know the emit prefixes are a bit weird, but it's what we use for other options.

@josharian
Copy link
Contributor Author

josharian commented May 12, 2022

Can we put each enum on its own line for both methods?

We can, although for the Valid method the compiler will generate better code if we can make them part of a single case statement, so it can unify the return statements. (That's also the reason not to write the code return e == EnumA || e == EnumB || ... || e == EnumX.)

What do you think about this:

func (e BooksBookType) Valid() bool {
	switch e {
	case BooksBookTypeFICTION,
                 BooksBookTypeNONFICTION:
		return true
	}
	return false
}

It's also not too hard to put (say) 4 items on a line, which might be nicer yet. WDYT?

@kyleconroy
Copy link
Collaborator

I think it's more straightforward to put one on each line. The comma style with a single case statement looks good to me.

@josharian
Copy link
Contributor Author

OK, there's now one per line. Go's templates sure don't make that easy.

This is now gated by config parameters. There's a surprising amount required to add a config parameter; I hope I didn't miss anything.

There is also an additional commit to make debugging regeneration failures easier to diagnose. (I'd like to do https://twitter.com/commaok/status/1345977948453490688 as well, but this is a good first start.)

This commit adds a Valid method for enum types
and an All...Values function to get all enum values.

This makes it easier to work with enums.

These are gated by new config parameters
emit_enum_valid_method and emit_all_enum_values.

Fixes sqlc-dev#1607
@jakoguta
Copy link
Contributor

The test data emit_enum_valid_and_values added is for PostgreSQL. I recommend we put them in the folder postgresql/pgx.

@josharian
Copy link
Contributor Author

The test data emit_enum_valid_and_values added is for PostgreSQL. I recommend we put them in the folder postgresql/pgx.

Genuine question: Does it matter? This codegen change is DB-agnostic.

@josharian
Copy link
Contributor Author

Ping.

What expectations should I have for review and issue comment turnaround time? It’d be useful to know so that I can plan accordingly and also not pester pointlessly.

Copy link
Collaborator

@kyleconroy kyleconroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small issue in the regen script, going to merge anyways

if _, err := os.Stat(filepath.Join(cwd, "stderr.txt")); os.IsNotExist(err) && failed != nil {
return fmt.Errorf("%s: sqlc-dev generate failed", cwd)
return fmt.Errorf("%s: sqlc-dev generate failed\n%s", cwd, out)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out is a byte slice, so I think this will print out gibberish.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the verb were %v then it would. With %s it prints as a string.

@kyleconroy kyleconroy merged commit 9c3a985 into sqlc-dev:main May 23, 2022
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

Successfully merging this pull request may close these issues.

autogenerate enum validation methods
3 participants