-
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
Properly implement import public
.
#329
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -295,6 +295,9 @@ exportWith q = Syntax.EThingWith () | |
q | ||
. map (Syntax.ConName ()) | ||
|
||
exportModule :: ModuleName -> ExportSpec | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, it's not intended for use outside of this project. Also, hopefully soon it will be replaced by |
||
exportModule = Syntax.EModuleContents () | ||
|
||
type Name = Syntax.Name () | ||
|
||
type Pat = Syntax.Pat () | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,8 +15,6 @@ import qualified Proto.Enum as Enum | |
import qualified Proto.Imports as Imports | ||
import qualified Proto.ImportsDep as ImportsDep | ||
import qualified Proto.Nested as Nested | ||
import qualified Proto.ImportsTransitive as ImportsTransitive | ||
import qualified Proto.ImportsTransitive2 as ImportsTransitive2 | ||
import qualified Proto.Google.Protobuf.Compiler.Plugin as Plugin | ||
import qualified Proto.Google.Protobuf.Descriptor as Descriptor | ||
|
||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is the right place for these tests since they're intended to test the effects of 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. |
||
-- modules in both imports_transitive.proto and (transitively) | ||
-- imports_transitive2.proto. | ||
testField @Imports.UseDep @ImportsDep.DepPkg #foo | ||
testField @Imports.UseDep @ImportsTransitive.TransitiveDep #bar | ||
testField @Imports.UseDep @ImportsTransitive2.TransitiveDep2 #baz | ||
testField @Imports.UseDep @ImportsDep.TransitiveDep #bar | ||
testField @Imports.UseDep @ImportsDep.TransitiveDep2 #baz | ||
|
||
testUseBootstrapped :: Test | ||
testUseBootstrapped = testCase "testUseBootstrapped" $ do | ||
|
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.
Should we draw attention to potentially breaking changes in some manner?
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.
I made a separate "breaking changes" section, like we did for 0.5.0.0.