gabrieldemarmiesse / python-on-whales

An awesome Python wrapper for an awesome Docker CLI!
MIT License
557 stars 102 forks source link

Support podman in test_image.py, improve parameterisation/marking mechanism #525

Closed LewisGaul closed 9 months ago

LewisGaul commented 9 months ago

More test parameterisation to come (in separate PR)...

LewisGaul commented 9 months ago

Having solved these concerns I'm now much happier with the setup of explicit parameterisation:

LewisGaul commented 9 months ago

could you split it into multiple pull requests if that's not too much to ask? I would be able to review much faster and also, it makes it easier to spot bugs/design errors.

While I agree with the sentiment, it's a trade-off between ease of development and ease of reviewing... I've tried to mitigate the size of the review by listing the changes in the description. The real development cost isn't putting it in different PRs, it's having multiple branches based on each other and keeping track of those dependencies, including my work on further parameterisation (I currently have a podman-tests3 branch based on this podman-tests2 branch...).

I can go ahead with splitting things up this time as I'm happy to have thorough reviews, but in general this is slowing things down and adding mental overhead for me and that might be something to consider.

Also part of the reason for grouping stuff is it makes it clearer how it all fits together, e.g. you have the whole test_image.py file to see how the parameterisation works.

gabrieldemarmiesse commented 9 months ago

Well I understand your position, I'm in the same position when I write a lot of code myself. I'm just not confortable reviewing a 400 LOC pull request. The quality of the code review will drop. If you don't want to split it (that's ok), I can split it myself and merge those pull requests. To be clear, it's a matter of making the review faster, but also a matter of ensuring there are no bugs in the modified code.

LewisGaul commented 9 months ago

Closing as not intended to be merged, can still be used for reference/comments.

See https://github.com/gabrieldemarmiesse/python-on-whales/pull/527 and the PRs dependent on that one (all mentioned and therefore linked).