Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Make typer an optional dependency. #513

Closed

Conversation

DanCardin
Copy link
Contributor

@DanCardin DanCardin commented Dec 11, 2023

Fixes #512

@DanCardin
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

DanCardin commented Dec 11, 2023

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
Copy link
Owner

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 pushed a commit that referenced this pull request Oct 13, 2024
Removes dependency on typer/tqdm/requests by dropping support for
downloading the docker client.

Initially agreed at
#512 (comment),
this has been deprecated since
#577 (v0.71.0
released in April 2024), and agreed we can now remove at
#552 (comment).
If accepted, this supercedes
#513 as a
resolution to
#512. We
will also be able to close
#575.
@gabrieldemarmiesse
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make typer/tqdm/requests optional dependencies?
3 participants