search

gabrieldemarmiesse / python-on-whales

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

refactor: Make typer an optional dependency. #513

Closed DanCardin closed 3 weeks ago

DanCardin commented 11 months ago

Fixes https://github.com/gabrieldemarmiesse/python-on-whales/issues/512

DanCardin commented 11 months ago

Ideally I would add to this that tqdm is also part of the "cli" extra. perhaps also add a progress=False flag (which defaults to False if tqdm were not installed), but the CLI defaults to True. In my opinion, the vanilla calling of python_on_whales through code shouldn't be enabling tqdm progress bars by default (except through the CLI). As a library, this sort of output is very surprising.

However, I didn't include that so far given your comments in the original issue.

gabrieldemarmiesse commented 11 months ago

Thank you for the pull request. I didn't expect it to be that fast! I think that first we need to take into account backward compatibility. By merging this right now, everyone relying on the CLI will have their CI pipeline broken. We should warn them first, for a few releases, then set typer as an optional dependency. Suddenly adding a breaking change to the software is not something that we should do unless there is a security issue.

Let's leave this PR open, we can merge it in a few releases. If you're willing to add the warning, I would happily merge such a PR now.

DanCardin commented 11 months ago

The problem, as I see it, with warning; is that I dont see that there's anything the user can do to make the warning go away. I'd perhaps argue that emitting a warning that a user can't do anything to resolve is no better than just doing the rug-pull more directly 😬.

I could add the extra, but leave the dependency as required. But as far as I know, there's no way to detect if a library was installed with an extra or not, so they'll get the warning even if they have switched to using the cli extra.

I would think use of the CLI would primarily be a local-use tool, rather than something that would break someone's build. but that's completely hearsay from my own personal use.

Also it occurs to me that if you were ultimately willing to make tqdm optional also, this PR would be the time to do it; so that there's only one "breaking" change visa vi the extra. I maintain per my previous comments that progress bars are very much a CLI-tool feature.

gabrieldemarmiesse commented 11 months ago

You are totally right. I didn't think that the check for the warning would be that complicated.

I'm really sorry about all the back and forth, I think I need more time to think about it. Breaking compatiblity so suddently is going to annoy many many users so I need to weight the pros and cons.

gabrieldemarmiesse commented 3 weeks ago

I think this can be closed as typer is not a dependency anymore! 🎉