botify-labs / simpleflow

Python library for dataflow programming.
https://botify-labs.github.com/simpleflow/
MIT License
68 stars 24 forks source link

Move 'swf' module into 'simpleflow.swf.mapper' #421

Closed jbbarth closed 11 months ago

jbbarth commented 1 year ago

This PR moves the "swf" module into the "simpleflow" one. The primary reason is to simplify imports and avoid the issues described in https://github.com/botify-labs/simpleflow/issues/420:

With everything in a single module I don't have circular dependencies anymore. An basic test can be found here: https://gist.github.com/jbbarth/5dcafe3a1c3041997e498f02e77af4c0

This is a big breaking change for simpleflow users since former imports won't work anymore, but it's fairly easy to fix (this is why I move everything as is in a single place).

Wdyt @ybastide? Should we proceed with that? Should we re-add a dummy "swf" module that only imports from "simpleflow" as a proxy?

ybastide commented 1 year ago

Hey, great! 🙂 With my user hat on, I guess rg '^import swf|^from swf' **/*.py will find the affected files in our codebase... docs/src/features/swf_layer.md should also be updated, shouldn't it?

ewjoachim commented 1 year ago

(I'd guess we could still leave a swf module that would do from simpleflow.swf.mapper import *)

ybastide commented 12 months ago

(I'd guess we could still leave a swf module that would do from simpleflow.swf.mapper import *)

Yes, unless it causes circular imports again; @jbbarth?

jbbarth commented 12 months ago

It won't imho, but it's not enough. If you try to import from a submodule, like I do in my code, it breaks:

simpleflow% mkdir swf/
simpleflow% echo 'from simpleflow.swf.mapper import *' > swf/__init__.py
simpleflow% python -c 'from swf.models import WorkflowExecution'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
ModuleNotFoundError: No module named 'swf.models'

So we would need to reproduce the whole module structure. And I don't think we have so many users that it's actually needed, let's just fix the few imports on the actively maintained codebases that use simpleflow? 🙈

ybastide commented 12 months ago

Agreed; what do you think, Joachim?