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

Circular dependencies hell #420

Closed
jbbarth opened this issue Aug 20, 2023 · 3 comments
Closed

Circular dependencies hell #420

jbbarth opened this issue Aug 20, 2023 · 3 comments

Comments

@jbbarth
Copy link
Collaborator

jbbarth commented Aug 20, 2023

I'm often hitting circular dependencies errors, and some colleagues have been as well. The nasty thing is that two persons can experiment different failures (things work for me, but not for my neighbour, although they have the same pyenv/python/libs/OS 😬 ; I don't know why).

Those issues are due to:

  • "swf" and "simpleflow" modules importing each other, notably for utils
  • imports happening in and from "init" files
  • not enough layering (?)

One trivial case to reproduce:

# happens on my machine (MacOS + Cpython 3.8.6)
# reproducible with "docker run -it python bash" + "pip install simpleflow"
python -c 'import swf.core'
# => ImportError: cannot import name 'ConnectedSWFObject' from partially initialized module 'swf.core' (most likely due to a circular import) (/usr/local/lib/python3.11/site-packages/swf/core.py)

I've just read this article and what they propose makes sense + they have an AST based test generator that allows to detect violations of the pattern they propose
=> I will take a shot at this to reorganise some imports, and move the remaining circular deps (if any) to local imports.

cc @ybastide let me know if you're against that

@ybastide
Copy link
Contributor

Definitively ❤️, thanks!

@jbbarth
Copy link
Collaborator Author

jbbarth commented Aug 25, 2023

First PR here: #421 -> moves "swf" into "simpleflow.swf.mapper" and solves the circular imports. I believe this PR is mandatory for fixing circular imports reliably.

Second PR here: #422 -> uses the set of tests in the article I mentioned above and rewrites the imports accordingly. Not sure it should be merged, curious what you think.

@jbbarth
Copy link
Collaborator Author

jbbarth commented Sep 6, 2023

Fixed by #421 and #422

@jbbarth jbbarth closed this as completed Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants