-
Notifications
You must be signed in to change notification settings - Fork 108
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
Proto3 enums #137
Proto3 enums #137
Conversation
This change removes support for `ghc-7.10`. The feature we're using was only added in `ghc-8.0.1`, and there are already a few other shims for that older version of GHC. This change uses explicit export lists in the generated proto modules (but only for `Proto.Foo.Bar`, not `Proto.Foo.Bar'Fields`). The same names should be exported as before, the only difference is that for enum aliases: ``` enum Foo { option allow_alias = true; Foo = 1; Bar = 2; BarA = 2; } ``` which produce the Haskell code: ``` data Foo = Foo | Bar pattern BarA :: Foo pattern BarA = Bar ``` we now export explicitly `Foo(.., BarA)`, rather than exporting `BarA` as a separate name. That way, if someone writes `import Proto.File (Foo(..))` then they also get `BarA` automatically.
For example: ``` enum Foo { Foo0 = 0; Foo1 = 1; } ``` becomes ``` newtype Foo = Foo Int32 pattern Foo0 :: Foo pattern Foo0 :: Foo 0 pattern Foo1 :: Foo pattern Foo1 :: Foo 1 ``` Additionally, proto3 enums are exported from the `Proto.*` module with the form `Foo(Foo0, Foo1)`, so that they can be imported as `Foo(..)` to get the patterns.
i think you want the description to be pattern Foo0 :: Foo
pattern Foo0 = Foo 0 |
-- TODO: Also export public imports, taking care not to | ||
-- cause a name conflict between field accessors. | ||
Nothing) | ||
(Syntax.ExportSpecList () <$> exports)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the reason for this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was part of #136 (now merged). Sorry for the noise.
Previously the modules that we generated always exported everything via module M where
, so the export list parameter was always Nothing
. Now, for the types module we need to pass an explicit list, i.e., module M (...) where
instead, so it can export pattern synonyms in a nicer way.
] | ||
++ | ||
-- pattern Value0 :: Foo | ||
-- pattern Value0 = Foo 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<3
dataName = enumName info | ||
dataType = tyCon $ unQual dataName | ||
|
||
generateEnumDecls Proto2 info = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens to dependent haskell code if you change the proto version? would anyone ever do that? if so, do these separate representations cause any problems?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you change from proto2 to proto3 you could incomplete patterns, e.g. in
case MyEnum of
Foo -> True
Bar -> False
when the enum becomes an open type. But -Wall
or similar will warn about that case. And given that this is an explicit behavior difference between proto2/proto3, I think it's something you'd want to be careful about in any language (similar to, e.g., the handling of default values).
@@ -17,8 +17,10 @@ message Foo { | |||
Sub sub = 5; | |||
|
|||
enum FooEnum { | |||
option allow_alias = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
, testCase "enum values" $ do | ||
map toEnum [0, 3, 3] @=? [Foo'Enum1, Foo'Enum2, Foo'Enum2a] | ||
["Foo'Enum1", "Foo'Enum2", "Foo'Enum2", "toEnum 5"] | ||
@=? map show [Foo'Enum1, Foo'Enum2, Foo'Enum2a, toEnum 5] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is toEnum
the only way to construct lax enums?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're also exporting the newtype constructor itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is amazing! merge it and we begin to nurse the pain of proto3 enums.
For example: ``` enum Foo { Foo0 = 0; Foo1 = 1; } ``` becomes ``` newtype Foo = Foo Int32 pattern Foo0 :: Foo pattern Foo0 = Foo 0 pattern Foo1 :: Foo pattern Foo1 = Foo 1 ``` Additionally, proto3 enums are exported from the `Proto.*` module with the form `Foo(Foo0, Foo1)`, so that they can be imported as `Foo(..)` to get the patterns.
For example: ``` enum Foo { Foo0 = 0; Foo1 = 1; } ``` becomes ``` newtype Foo = Foo Int32 pattern Foo0 :: Foo pattern Foo0 = Foo 0 pattern Foo1 :: Foo pattern Foo1 = Foo 1 ``` Additionally, proto3 enums are exported from the `Proto.*` module with the form `Foo(Foo0, Foo1)`, so that they can be imported as `Foo(..)` to get the patterns.
Implement proto3-style "open" enums.
For example:
enum Foo { Foo0 = 0; Foo1 = 1; }
becomes