Add an option to handle enums with ugly casing #270

benjaminjkraft · 2023-05-06T18:46:08Z

There are a bunch of places in genqlient where we just kind of hope you don't have ridiculous casing conflicts in your schema. Apparently with enum values there are actual schemas that have this problem! Now we have an option to disable. I ended up putting it all under casing instead of in bindings so we can clearly document the list of algorithms we support, and so we can have an all_enums value if you're working with a schema with a lot of this; in the future we may want to add similar behavior for types/fields, add more possible algorithms (e.g. to make things unexported), etc.

I added tests for the new feature, although the way the tests are set up it wasn't convenient to do so for a schema where this is actually required. I also added a check for case conflicts that points you to this option. (I don't want to do it automatically for reasons described in the issue; mainly it just seemed a bit too magical.)

Fixes #265.

I have:

Written a clear PR title and description (above)
Signed the Khan Academy CLA
Added tests covering my changes, if applicable
Included a link to the issue fixed, if applicable
Included documentation, for new features
Added an entry to the changelog

There are a bunch of places in genqlient where we just kind of hope you don't have ridiculous casing conflicts in your schema. Apparently with enum values there are actual schemas that have this problem! Now we have an option to disable. I ended up putting it all under `casing` instead of in `bindings` so we can clearly document the list of algorithms we support, and so we can have an `all_enums` value if you're working with a schema with a lot of this; in the future we may want to add similar behavior for types/fields, add more possible algorithms (e.g. to make things unexported), etc. I added tests for the new feature, although the way the tests are set up it wasn't convenient to do so for a schema where this is actually required. I also added a check for case conflicts that points you to this option. (I don't want to do it automatically for reasons described in the issue; mainly it just seemed a bit too magical.) Fixes #265.

csilvers

Whew! A lot of machinery (and testing!) for a fairly contained change. But it all looks right to me.

I take it you're reserving the option to control casing for other things besides enums in the future, but don't feel the need to implement that yet.

csilvers · 2023-05-07T14:51:52Z

generate/convert.go

@@ -516,8 +516,23 @@ func (g *generator) convertDefinition(
 			Description: def.Description,
 			Values:      make([]goEnumValue, len(def.EnumValues)),
 		}
+		goNames := map[string]*goEnumValue{}


I'm sure it doesn't matter, but maybe make(map[string]*goEnumValue, len(def.EnumValues))?

csilvers · 2023-05-07T14:55:56Z

generate/types.go

@@ -143,8 +144,7 @@ func (typ *goEnumType) WriteDefinition(w io.Writer, g *generator) error {
 	for _, val := range typ.Values {
 		writeDescription(w, val.Description)
 		fmt.Fprintf(w, "%s %s = \"%s\"\n",
-			typ.GoName+goConstName(val.Name),
-			typ.GoName, val.Name)
+			val.GoName, typ.GoName, val.GraphQLName)


Here's where all that machinery pays off!

benjaminjkraft

Yeah pretty much. Some of the testing machinery is already useful for other things (that aren't really getting tested well) but some of this is looking forward to #206, #18, etc.

I set these tests up in #270, but I realized there's a lot more we could test unrelated to that PR. In this commit I add some more tests. They're not really exhaustive yet, but they did catch a few bugs, which I fixed: - we weren't validating the package-name if you do set it (only if we guess it) - If you omitted `generated`, you would try to write generated code to the directory containing `genqlient.yaml`, which makes no sense; now we default to `generated.go`. In the real world it's probably good to set explicitly, but it's actually very convenient in tests that we don't have to, and maybe in small projects too.

partounian · 2023-05-24T23:20:45Z

By chance will this feature be released soon?

Oh I guess I can use go run github.com/Khan/genqlient@main

benjaminjkraft · 2023-05-24T23:32:30Z

Yep, we usually let new features bake a bit before we cut a release in case folks run into bugs in the real world that aren't covered in our tests, but if you want them right away then you can just use the main branch!

benjaminjkraft requested review from csilvers, dnerdy and StevenACoffman as code owners May 6, 2023 18:46

appease gocritic

0069ed3

benjaminjkraft mentioned this pull request May 6, 2023

Field name tranformations #206

Open

csilvers approved these changes May 7, 2023

View reviewed changes

benjaminjkraft commented May 7, 2023

View reviewed changes

review comment

1c0aa33

benjaminjkraft merged commit ff790e1 into main May 7, 2023

benjaminjkraft deleted the benkraft.enum-casing branch May 7, 2023 19:15

benjaminjkraft mentioned this pull request May 7, 2023

Add some more tests for config validation, and fix some gaps #274

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an option to handle enums with ugly casing #270

Add an option to handle enums with ugly casing #270

benjaminjkraft commented May 6, 2023

csilvers left a comment

csilvers May 7, 2023

csilvers May 7, 2023

benjaminjkraft left a comment

partounian commented May 24, 2023 •

edited

Loading

benjaminjkraft commented May 24, 2023

Add an option to handle enums with ugly casing #270

Add an option to handle enums with ugly casing #270

Conversation

benjaminjkraft commented May 6, 2023

csilvers left a comment

Choose a reason for hiding this comment

csilvers May 7, 2023

Choose a reason for hiding this comment

csilvers May 7, 2023

Choose a reason for hiding this comment

benjaminjkraft left a comment

Choose a reason for hiding this comment

partounian commented May 24, 2023 • edited Loading

benjaminjkraft commented May 24, 2023

partounian commented May 24, 2023 •

edited

Loading