pypa / cibuildwheel

🎡 Build Python wheels for all the platforms with minimal configuration.
https://cibuildwheel.pypa.io
Other
1.78k stars 227 forks source link

Split file utils into dedicated submodule #1873

Open MusicalNinjaDad opened 2 weeks ago

MusicalNinjaDad commented 2 weeks ago

This is a first step towards #1857 designed to get feedback and validate the approach.

Considerations

  1. Don't make a breaking change: anyone currently using from cibuildwheel.util import *, from cibuildwheel.util import foo or import cibuildwheel.util should not notice the change. This has driven the implementation of util/__init__.py
  2. Split by family: grouping functions based on the domain that they support rather than keeping functions together that rely on each other. This means we will be importing between util submodules - via 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.
  3. Provide insights to developers in IDE: prefer docstrings to inline comments to explain the "what" as these are rendered on mouse-over etc. Use comments to explain the "why" behind specfic design / implementation decisions

Checks performed

Relying on the pipeline to perform more extensive integration testing

MusicalNinjaDad commented 1 week ago

@mayeut , @henryiii - does this look like the right sort of approach for splitting out utils? I've detailed my thought patterns in the description and would welcome your insights.

mayeut commented 1 week ago

IMHO, we don't need the first item. cibuildwheel is not a library so this module is internal. I would move download to files.py

henryiii commented 12 hours ago

I think the next step is correcting the imports so we can avoid import *.