Skip to content
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

Import bug - Circular Dependencies #100

Conversation

boukeversteegh
Copy link
Collaborator

@boukeversteegh boukeversteegh commented Jul 4, 2020

Protobuf itself does not allow circular dependencies (import "a.proto" in b.proto, and import "b.proto" in a.proto).

So why do we still get circular import errors in python? Because protobuf dependencies are on file-level, while betterproto dependencies are on package-level.

Example:

The proto messages depend on each other in a non-circular way:

Test -------> RootPackageMessage <--------------.
  `------------------------------------> OtherPackageMessage

Test and RootPackageMessage are in different files, but belong to the same package (root):

(Test -------> RootPackageMessage) <------------.
  `------------------------------------> OtherPackageMessage

After grouping the packages into single files or modules, a circular dependency is created:

(root: Test & RootPackageMessage) <-------> (other: OtherPackageMessage)

Because we don't know in advance which imports will cause a circular-dependency (as we currently do single-pass compiling), the only solution I see is to always use forward references ("package.SomeType"), and have the import statements at the bottom.

This PR implements that solution.

@boukeversteegh boukeversteegh changed the title Fix/circular dependencies Import bug - Circular Dependencies Jul 4, 2020
@boukeversteegh boukeversteegh requested a review from nat-n July 4, 2020 13:54
@boukeversteegh boukeversteegh added this to the Better Imports milestone Jul 4, 2020
@boukeversteegh boukeversteegh added bug Something isn't working has test Has a (xfail) test that verifies the bugfix or feature labels Jul 4, 2020
@boukeversteegh boukeversteegh merged commit 3273ae4 into danielgtaylor:master Jul 7, 2020
@abn abn mentioned this pull request Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working has test Has a (xfail) test that verifies the bugfix or feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants