Repository navigation
More private utilities that need to be marked and public api deprecated #3847
Description
Activity
Should
open_fileandsafecallbe made private as well?I'm looking at adapting
pip-toolsto handle v8.5.0 this morning, and we have (this all looks weird to me)...my_file = click.open_file(filename, "w+b", atomic=True, lazy=True) assert my_file is not None if isinstance(my_file, click.utils.LazyFile): ctx.call_on_close(click.utils.safecall(my_file.close_intelligently))
I don't see any reason that anyone outside of
clickshould ever usesafecall: it's trivial to write it yourself.open_filedoesn't seem good to expose if its return type won't be exposed. It's hard to figure out how to use it correctly in this case.(We do want
close_intelligently()since the target filename could be"-".)safe call is deprecated. It should issue a deprecation warning. I will have to check on the other.
We will have to look at open_file. It is public on purpose but may need better type annotation or docs.
I didn't want to suggest too much at first, but maybe we can spec it out with a
typing.Protocol? I'm happy to help out if you want to go that route.And yeah, I didn't notice that
safecallwas also warning, since I fixed this "all at once" on my end.Feel free to open an issue and propose a PR for what you think is best. I am always interested in type hints or docs that make usage clearer.
Reacted by Stephen Rosen and 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко)I feel like
open_fileshould be private, but I haven't looked enough.atomicshould be deprecated, if I remember correctly it's not actually atomic and there are better libraries out there for atomic access.lazyis only useful for theFileparam type when opening a file for writing, and should be invisible to the public API. It shouldn't matter if it's lazy, it should just close silently.Aside: I didn't suggest a protocol because I didn't come up with a good name or home for it. Which might be a sign that it's not a great idea.
I'm not aware of a good, supported library for atomic/transactional writes. So if it becomes private, I'll probably review this old thread (which has lots of good notes and ideas) and roll my own. Which I'm okay with.
From my perspective:
- it's weird that
open_fileis exposed at all; it feels orthogonal to most of the concerns ofclick(even though it will be a pain for me, I'm in favor of making it private) - it's arguable that even implementing
lazy=Trueis outside of whatclickshould be doing
I'm using
LazyFilein a few places today, but I feel like the ideal path is to put together something that's a replacement and can be passed as a param type, but outside ofclick. Let the core library stay lean and focused.- it's weird that
it feels orthogonal to most of the concerns of click
Yes, this is a general problem with the pallets libraries exposing so many utilities and customization points. We're trying to clean this up in general. See for example the next Werkzeug 3.2 release, which is a huge wall of deprecations.
Reacted by Stephen RosenThe library I was thinking of was atomicwrites, which is archived in favor of
os.replaceoros.rename. Those are both documented as atomic.The simplest answer for lazy is probably to use
Pathinstead ofFileparam types, and handle opening the file yourself when you write. That's probably a lot simpler than all the stuff we have to go through forFileanyway.Reacted by Stephen Rosen
In types.py
convert_type- rename to _convert_type - currently has no top level importFuncParamType- maybe typing thing? - currently has no top level importIn _ ?,