Conversation
8d05bcd to
9e44aa4
Compare
bhrutledge
left a comment
There was a problem hiding this comment.
This might be pedantic, but "wheel" seems worth defining.
|
Also, FWIW, I think it would have been okay to add this to #910. |
Co-authored-by: Brian Rutledge <brian@bhrutledge.com>
I've been told before by @webknjaz it's better to keep things separate, so I've tried to do that. If one part blocks, the other can still go in. |
bhrutledge
left a comment
There was a problem hiding this comment.
it's better to keep things separate,
Agreed. In this case, though, it seems like these changes are tightly-coupled to the changes in #910, and IMHO would be more easily reviewed together. I don't think that would fall under @webknjaz's qualifications for breaking it up:
I find it easier to review/approve separate unrelated changes so if one of the updates wouldn't block others from being merged. Usually when the title contains references to many things, has "and" in it, or is too vague within the 50 char limit, it's a sign that the PR is too broad and when you can describe its separate parts with a few specific titles, it's a good idea to have a PR per such a title.
But I'm just being nit-picky while learning the ropes as a gardener for this project. 😄
IMO, this is a balancing act. It's possible to go too-far with breaking up PRs -- having a bias toward smaller is good (easier to review + iterate), as long as there's no dependency between the various PRs. If there's dependencies in the changes, it's much better to just combine the changes into a single PR that can be reviewed as a whole. |
pradyunsg
left a comment
There was a problem hiding this comment.
Thanks @henryiii for the PR and @bhrutledge for the thoughtful reviews! ^>^
This adds multibuild, pre-req for #910. CC @matthew-brett - better description welcome (we don't have much of a description on cibuildwheel either, @pypa/cibuildwheel-team).