Properly implement import public.
#329
Conversation
Fixes #19. After this change, if `bar.proto` contains the line import public foo.proto Then it turns into ``` module Proto.Bar(..., module Proto.Foo) where import Proto.Foo -- unqualified, to allow the reexport of its definitions ``` The previous hacky approach was: `Proto.Bar` doesn't reexport `Proto.Foo`; instead, if another file `baz.proto` imports `bar.proto`, *then* the codegen of `Proto.Baz` imports `Proto.Foo` as well as `Proto.Bar`. This approach let us avoid module-level exports, but had two limitations: 1) It violated the principle of "strict dependencies", since `Proto.Baz` is importing what is essentially a transitive dependency. 2) Non-codegen modules that import a proto module can't benefit from `import public` statements.
There was a problem hiding this comment.
I'll look at the code in a bit, for now some thoughts on the social aspects.
Could you add an entry to the ChangeLog describing this change? Maybe also mention on reddit and some Haskell mailing lists?
I imagine we can try this internally pretty easily. Do we have any users in Formation who could also try the change before the release?
|
I can post an announcement to the Haskell list/reddit when we release the new version. We have a few changes queued up that will require a major bump; we can batch them together. I'll also see if we can get Formation to try out the new version before we release it. |
|
Updated the changelog. |
proto-lens-protoc/Changelog.md
Outdated
| @@ -3,6 +3,8 @@ | |||
| ## Pending | |||
| - Fix a potential naming conflict when message types and enum values | |||
| are the same except for case. | |||
| - Reexport transitive definitions from files with `import public` statements | |||
There was a problem hiding this comment.
Should we draw attention to potentially breaking changes in some manner?
There was a problem hiding this comment.
I made a separate "breaking changes" section, like we did for 0.5.0.0.
| @@ -295,6 +295,9 @@ exportWith q = Syntax.EThingWith () | |||
| q | |||
| . map (Syntax.ConName ()) | |||
|
|
|||
| exportModule :: ModuleName -> ExportSpec | |||
There was a problem hiding this comment.
We haven't been particularly good about documenting this module, is this because it's not supposed to be user facing?
There was a problem hiding this comment.
Yeah, it's not intended for use outside of this project. Also, hopefully soon it will be replaced by ghc-source-gen.
| @@ -46,9 +44,12 @@ testFoo = testCase "testFoo" $ do | |||
|
|
|||
| testUseDep :: Test | |||
| testUseDep = testCase "testUseDep" $ do | |||
| -- Due to "import public" statements, imports_dep.proto reexports the | |||
There was a problem hiding this comment.
One of the existing tests seems to be detecting a difference suggesting the code is achieving the goal. Does it also make sense to add a new test just for this feature?
There was a problem hiding this comment.
I think this is the right place for these tests since they're intended to test the effects of import public.
I changed this a little so it keeps the previous test cases and just adds new ones (showing the new behavior) rather than replacing them.
Fixes google#19. After this change, if `bar.proto` contains the line import public foo.proto Then it turns into ``` module Proto.Bar(..., module Proto.Foo) where import Proto.Foo -- unqualified, to allow the reexport of its definitions ``` The previous hacky approach was: `Proto.Bar` doesn't reexport `Proto.Foo`; instead, if another file `baz.proto` imports `bar.proto`, *then* the codegen of `Proto.Baz` imports `Proto.Foo` as well as `Proto.Bar`. This approach let us avoid module-level exports, but had two limitations: 1) It violated the principle of "strict dependencies", since `Proto.Baz` is importing what is essentially a transitive dependency. 2) Non-codegen modules that import a proto module can't benefit from `import public` statements.
Fixes #19.
After this change, if
bar.protocontains the lineThen it turns into
The previous hacky approach was:
Proto.Bardoesn't reexportProto.Foo;instead, if another file
baz.protoimportsbar.proto, then the codegenof
Proto.BazimportsProto.Fooas well asProto.Bar. This approachlet us avoid module-level exports, but had two limitations:
Proto.Bazis importing what is essentially a transitive dependency.
import publicstatements.