Split file utils into dedicated submodule#1873
Split file utils into dedicated submodule#1873MusicalNinjaDad wants to merge 11 commits intopypa:mainfrom
Conversation
for more information, see https://pre-commit.ci
|
IMHO, we don't need the first item. cibuildwheel is not a library so this module is internal. |
|
I think the next step is correcting the imports so we can avoid |
|
Hey @MusicalNinjaDad ! Thanks for putting this together. However, I must say I'm not totally convinced by the need for this change. Admittedly, the utils module is large, but utils are generally self-contained chunks of code that have very few interdependencies - i.e. they're simple. But happy to be outvoted, perhaps I'm missing some extra context here, please let me know if so - I've been out of the project for a few weeks! (I think perhaps this started from a problem with circular imports. Regarding that, utils should never import another module in the codebase, with the possible exception of a file of basic types or constants or something.) Having said that, your points around docstrings are valid, and a bit of effort here could improve things for sure. |
|
IMO, having a bit more separation here would improve readability and one massive "utils" file for everything is not ideal. (See various articles on this, like https://breadcrumbscollector.tech/stop-naming-your-python-modules-utils/). I think it will help when this PR is closer to a final form, without the |
|
This could also be done as |
The goal would be that |
|
@joerick , I agree this is very much a style question and not a must-do. As a new contributor I personally found that The original need was driven by a circular import with For that reason I'm happy to invest time on this change, if it is considered worthwhile, or, at least, harmless and possibly useful. |
MusicalNinjaDad
left a comment
There was a problem hiding this comment.
IMHO, we don't need the first item. cibuildwheel is not a library so this module is internal.
I would movedownloadtofiles.py
Done & done :)
642a9dc to
ea7577d
Compare
|
I think that the change is either going to be worth it done at once, or not at all. Here's a go at how we might organise things- how does this feel to people? I think that could work. I don't think there are any circular dependencies there, but that is a concern with breaking the file up. |
This is a first step towards #1857 designed to get feedback and validate the approach.
Considerations
Don't make a breaking change: anyone currently usingBased on comments below by @mayeut & @henryiii ... Expectfrom cibuildwheel.util import *,from cibuildwheel.util import fooorimport cibuildwheel.utilshould not notice the change. This has driven the implementation ofutil/__init__.pyfrom util import xto be explicit in future: Adjust the relevant imports within the codebase while keeping__all__in place for cases wherefrom utils import *is in use.from util.submodule import bar. To me it feels like it is more likely to find valid and obvious fracture plains with this approach. Also if something is worth an independent function then it most likely either expects reuse in multiple contexts, abstracts away something that doesn't belong in the original flow but belongs somewhere else, or both.Checks performed
E0611(no-name-in-module) errorspytest unit_testpassesRelying on the pipeline to perform more extensive integration testing