Is changing case of common initialisms a bad design ?

[flintforge]

The list could go on forever, and it's not the job of the generator to stumble upon specific cases.
This is tied to a semantic context: for instance fields named cpu or ip may not be expected to be turned uppercase.
If I may, I would advice to let users provide themselves any list of strings they judge necessary to go through this transformation. Even the id case (be it Freudian or not, cf templates:449) is dubious IMHO.

[JonasDoe]

I agree on that, but I think I've got that covered in a - rather dusty - issue: #1383

[nettrino]

+1 to letting users define this. In our project we are generating go types from a schema shared with other servers and this casing change simply breaks the other consumers

[JonasDoe]

The issue is still bugging us. As a compromise, could you at least expose the information which "common initialisms" there are, so we could regard them in our code?

[StevenACoffman]

So continuing to use the existing list of initialisms as the default, but allowing overriding it with a config option is a great idea for a PR. I look forward to your contribution.
@JonasDoe

[JonasDoe]

@StevenACoffman Alright, I can have a look at it! Not really sure about the config option, though. Do you really want a potentially long list of acronyms to appear in the gqlgen.yml? It might bloat it.

[StevenACoffman]

@StevenACoffman Allright, I can have a look at it! Not really sure about the config option, though. Do you really want a potentially long list of acronyms to appear in the gqlgen.yml? It might bloat it.

Not for people who like the current defaults! :) https://github.com/99designs/gqlgen/blob/master/codegen/templates/templates.go#L598-L642 has 44 lines if someone wanted to add that many config lines to entirely replace that default list (plus however many any new ones). If that config option is at the end of the gqlgen.yml file, I don't think it will be much of a burden to readability.

[JonasDoe]

I can imagine some ppl. want want to add something to these 44 lines, creating a list with > 44 entries in the config. But maybe that could get prevented with an option like replace_defaults which by default causes the list to get appended to the default config instead of overriding the defaults. 🤔 This would also ensure that if the value is not set nothing changes, b/c it meant "append an empty list to the default config".

[StevenACoffman]

Sure, but any one project is unlikely to desire more than a handful of intialisms. If you look at that list, sure, some projects might actually have an entity with "HTML" or "XSS", but most will not. That's true of most of the items on the list. It's just that everyone has a different set.

However, I'm fine with whatever you come up with that doesn't break those

[JonasDoe]

Well, I'm facing a problem here: Since I need to pass this new config infos to methods templates.ToGo(...), this will break all plugins. I mean, I can adjust the bundled Federation plugin, but I can't adjust the whole ecosystem. Similar with func Funcs() template.FuncMap, which contains ToGo as well. This is all determined by compile-time, rendering it hard to squeeze in some runtime adjustments. Imo, the whole template package should be stateful (i.e. coming with a struct whose methods are ToGo(...) etc., but that would break with every plugin out there.

Since I can't pass any config to those methods, the only "non-breaking" solution I see it to load the relevent data from the config file in templates.go again (e.g. with init()) and store it in a global variable, so it can be regarded by the ToGo function etc. But that's really ugly and wonky for many reasons - what's your opinion about that?

Something different I've stumbled upon: imo it's not ideal that the config module depends on the templates module (due to a single line in binder.go) - I think the other way around made more sense. In its current state, it's prone to circular imports.

[StevenACoffman]

Yeah, I noticed the potential for circular dependency before. It hasn't been enough of a problem (There's one or two other similar situations) but it was easy enough that I made #2682 and merged it to fix that wart.

Currently the commonInitialisms is already a package var so changing its value in an exposed public function is probably the most expedient, and you can just no-op if that config option is not set to do anything. I'm not sure you have the config handily loaded and available to you inside an existing init() that I'm aware of, but during the config read of your new initialisms would be an appropriate time to modify the package var commonInitialisms, correct?