Closed dmitshur closed 7 years ago
gmlewis
commented
7 years ago Thanks for the detailed descriptions. Here are my $0.02:
I like Solution 4 the best, as it seems like the cleanest one to me. My second-favorite is Solution 3, with the downside of inconsistent initialisms. My 3rd-favorite is Solution 1, with the downside of Java-like (long) names. My least favorite is Solution 2, as getting those unicode dots typed can be a pain and frustrating.
bradleyfalzon
commented
7 years ago I'm not a fan of Solution 4 for two reasons:
1) Introduces a risk of import cycles, even though githubql may never import reactioncontent, this pattern may be used for other types where import cycles could be a problem.
2) Although the design shouldn't be determined by the limitations of editors/IDEs, many (but not all: goglang) can only autocomplete for imported packages.
Although Solution 4 is similar to how stripe-go structures itself, so I don't think it's an unusual solution.
I'm not a fan of Solution 3, mostly because the initialisms may have a high chance of collisions, is PStateOpen Project Open or does P mean something else? I believe my reservations on this are similar to @gmlewis's, or I'd also agree with him on the inconsistencies.
Solution 2 (unicode dot) I don't think really brings too much more value over Solution 1, and also it could be difficult to type (although, one could argue it could be a different symbol).
Therefore I think Solution 1 is my favourite, autocomplete works (as the package is probably being imported already), there's no import cycle risk, it's easily discoverable reading docs (godoc.org separates constant blocks well) and just searching works for "reaction" or "thumbs".
Edit: By import cycles risk, I mean, if githubql needs to return one of these types, perhaps in a response or in a helper. For example, you query for an issue the issue has a thumbs up reaction, githubql can't return the type reactioncontent.ThumbsUp because reactioncontent uses githubql.ReactionContent. However, I'm not certain this would be a problem though.
tallstoat
commented
7 years ago I'd go for 3 if I know the different types of enums (and their possible values) upfront, which I guess should be the case here to a large extent. Next (or rather a safer approach though verbose) would be 1. Last option would be 4.
I wouldn't go anywhere near 2; that road is best avoided.
alicebob
commented
7 years ago Thanks for the interesting talk at the Berlin meetup this evening!
This is another option for these constants. I would likely not choose it, but I still want to mention it.
package main
var Reactions = struct {
ThumbsUp, ThumbsDown, Laugh string
}{
ThumbsUp: "THUMBS_UP",
ThumbsDown: "THUMBS_DOWN",
Laugh: "LAUGH",
}
func main() {
print(Reactions.Laugh)
}
If you want to add an extra layer of nesting you can't use the anonymous structs anymore:
package main
type Reaction struct {
ThumbsUp, ThumbsDown, Laugh string
}
type Colors struct {
Red, Blue, Green string
}
type ConstsT struct {
Reaction Reaction
Colors Colors
}
var Consts = ConstsT{
Reaction: Reaction{
ThumbsUp: "THUMBS_UP",
ThumbsDown: "THUMBS_DOWN",
Laugh: "LAUGH",
},
Colors: Colors{
Red: "RED",
Blue: "BLUE",
Green: "GREEN",
},
}
func main() {
print(Consts.Reaction.Laugh)
}
dmitshur
commented
7 years ago @alicebob, thanks for that suggestion. It's an interesting alternative that I haven't thought of, so it's good to be aware of it.
It has some benefits, and feels most similar to option 2, except without the need for hard-to-type unicode middots. However, it has some flaws, like the fact the enum values are no longer consts, and the name collision between the containing struct name and the enum type.
dmitshur
commented
7 years ago I've been thinking about this issue for a while, and I've decided to go with Solution 1 as a resolution for now, but be open to revisit this issue later. I need to resolve the issue so I can make progress on implementing next missing features.
Solution 1 is the simplest, most predictable, and least controversial option. Its only downfall are the verbose enum value identifiers, but it suffers from least other problems.
Since the API of the library is not frozen yet, I think it's a good step to make, and if we can find an even better next step to make, we still can. In fact, I won't close the issue for now, we can keep it open and keep thinking.
dmitshur
commented
7 years ago Some time has passed, and I want to follow up on this.
I've been using our current solution (Solution 1) and continuing to really like it. It's very simple, and feels good to use. The absolutely only downside of slightly higher than expected verbosity pales in comparison to the downsides of the other more involved alternative solutions we've considered above.
One property of the current naming I appreciate is that typing the enum name makes it easy to see what are the possible values via code completion:
It'd be helpful if others who have had a chance to use it posted their thoughts too.
However, at this point, I suspect the chance of another solution being discovered which would beat our current simple solution is extremely unlikely. (Of course, if it happens, we can still consider implementing it.) I'll wait some more, and then close this issue.
dmitshur
commented
7 years ago I'll wait some more, and then close this issue.
I've waited, nothing has come up. Closing now because this issue is considered resolved. (Can always be reopened if something new comes up.)
As discovered in #7, the initial schema I had in mind for the enums will not work, because of name collision between enum values of different types:
Solutions
These are the solutions that I've got so far and are up for consideration.
Solution 1
Prepend the type name in front of the enum value name, to ensure each identifier is unique and collisions are not possible:
Usage becomes:
This is very simple, guaranteed to not have collisions. But the enum value identifiers can become quite verbose, and less readable.
Solution 2
This is a minor variation of solution 1. The idea is to make the enum values slightly more readable by separating the type name and enum value by a middot-like character ۰ (U+06F0).
Usage becomes:
The unicode character is not easy to type, but easy to achieve via autocompletion:
Solution 3
This is also a variation of solution 1. The idea, suggested to me by @scottmansfield (thanks!), is to use an initialism of the type name rather than the full name. This makes the identifier names shorter, but still has a risk of there being name collisions if two different types with same initialism have same enum values.
Usage becomes:
Unfortunately, this approach led to a collision with the existing enums in GitHub GraphQL API:
It's possible to detect when a collision would occur, and use something more specific that doesn't collide, instead of the initialism. But that introduces complexity, and inconsistency/unpredictability in the naming.
Solution 4
This idea is somewhat related to solution 2, but instead of a hard-to-type character, it becomes a normal dot selector. The idea is to create a separate package for each enum type, and define its values in that package. The enum types are still in main
githubqlpackage.Usage becomes:
The dot character is easy to type, and the code is very readable once written. But this will require importing many new small packages when using enum values. A tool like
goimportswill make that significantly easier, but it may still be problematic. Also, the documentation will be split up into multiple small packages, which may be harder to read.Conclusion
All solutions considered so far seem to have certain upsides and downsides. I'm not seeing one solution that is clearly superior to all others. I'm considering going with solution 1 or 2 to begin with, and be open to revisit this decision.