Repository navigation
Better sorting & sorting out circular deps #290
Description
First let me start with a big thanks to @DavidVujic for maintaining this awesome library.
My team uses it and we love it. so much that it has become part of our CI and I even made a zsh plugin so it easier to poetry poly around lol (@DavidVujic ping me in private if you want to distribute it somehow, I don't mind sharing).
I have 2 features I would like to discuss:
Is your feature request related to a problem? Please describe.
-
when using ppc (poetry poly check) -v, each time the order of packages and the order that comes after "is importing" is changed. also for some reason there's a line break if the list exceeds some length, causing tools like
grepto be harder to apply. -
let's say I have 2 bricks, aa and bb. aa defines classes A and B and bb defines class C such that
# a.py
class A:
pass
# b.py
from proj.bb.core import C
class B(C):
pass
# c.py
from proj.aa.core import A
class C(A):
passwhich is perfectly python legal but results in :
which can lead to bad packages includes in the pyproject.toml file.
Describe the solution you'd like
- sort the packages and the part after "is importing" (lexicographically I assume?) and don't introduce line breaks
- make it so poetry poly check or another command if necessary will result in something like
🤔 Circular dependency detected between aa and bb
Error: Process completed with exit code 1.Describe alternatives you've considered
There's no alternative to polylith. it's really good.
Additional context
Keep up the good work David 👏
Activity
DavidVujic commentedon Nov 17, 2024
Thank you for reporting @talsuk5 and I am very happy to hear that you like Polylith 🤩
I am now working on a solution for
1.(in issue #291). The second one is a scenario I hadn't thought of before. I will do some experiments, try things out and find a solution as soon as the first issue is done.DavidVujic commentedon Nov 17, 2024
I have published a new version with the output is sorted, and the import sections (bricks and third-party) are no longer repeated/duplicated. I also added some coloring for the types of bricks, to make it easier to understand what kind of brick it is.
More details (with screenshots) in #292
I hope that this solves some of the issues. 😄 I haven't added any changes to line-breaks in this version, though. I'm using Rich, and the options I found are not working that well (some options are probably dependent on the type of shell).
I will continue looking into the circular references in
poly deps.DavidVujic commentedon Nov 17, 2024
I have tried out adding
aaand abbcomponents:the
aacomponent:The
bbcomponent:Polylith will recognize these ones as you write as depending on each other. I added the
aacomponent to one of the projects, and then ran thepoly synccommand. It added the missingbbcomponent as expected.In my example FastAPI project, I created an instance of one of the classes:
The app can be started and runs fine. So far there are no circular references in the python modules. Then I added an import to the
aa.bmodule:This should mean that there are circular references. The result when I try to run the app:
ImportError: cannot import name 'C' from partially initialized module 'example.bb.core' (most likely due to a circular import)I think this could be outside of the scope of Polylith maybe? The double referencing between
aaandbbis risky, and maybe Polylith should warn about this. I'm worried about the use cases where this kind of referencing is intentional, and Polylith reports about something that is not a problem for the team, even if alarm bells would ring in my head when seeing this kind of structure 😄I'm not sure how to proceed here, do you have any suggestions?
talsuk5 commentedon Nov 17, 2024
Thanks @DavidVujic.
So the result you're seeing is in runtime right?
In my case the circular dependency didn't break in runtime, but when working on some feature and importing an innocent models component, I discovered it was hiding some unholy deps.
Of course this is semantic and python being python will sometime "warn" you with a good runtime exception.
I absolutely agree it's not a must for polylith, but a nice to have feature that will upgrade this project IMO.
I could also argue that polylith is a semantic tool, hence the rules that apply to it are totally in the hand of its creators/maintainer.
The other alternative for me is to write a script that identifies this behavior and breaks the CI.
So I guess it's up to you (or me if I'll ever have the time lol) to decide if this feature is worth it.
DavidVujic commentedon Nov 17, 2024
Thank you for your input and ideas about how to improve the Polylith tool, much appreciated ⭐
I will give this some thought, and realize that I need to read up more on circular imports in Python. I will move this issue to Discussions and in the Ideas section.