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

DOCS/lua: note that properties are preferred to getcwd and getpid #13226

Closed
wants to merge 1 commit into from

Conversation

guidocella
Copy link
Contributor

This prevents script writers from getting confused over whether to use these functions or the properties, like in
#7975 (comment)

They are moved to the end of the list of utils functions because having the first one in the list be a deprecated one would be weird.

@po5
Copy link
Contributor

po5 commented Jan 5, 2024

02fbdf8

After some discussion on IRC it was decided that it's easier for us to
maintain them as trivial wrappers than to deprecate them and inflict
pain on users and script authors, so currently no plan to deprecate.

I know this is not yet gone, but it will be when the next round of blind deprecated functionality removal happens.
Please stop breaking existing scripts.

I'm sure someone thinks "please stop depending on deprecated features", but at least one PR #9541 has broken functionality in trackselect and I don't believe there is a way to reproduce the same behavior (detecting when a track is explicitly switched to by the user) by observing the track-list property.
It's not so much that there's been very little consideration for existing setups, but an active effort to break them for no benefit.

@guidocella
Copy link
Contributor Author

These will probably never be removed as there is no maintenance cost. The purpose of the deprecation is to avoid confusion. There are about man mpv | grep -c deprecated 44 more deprecated things that no one is removing.

@po5
Copy link
Contributor

po5 commented Jan 5, 2024

I know that it wasn't your PR, but to illustrate my point #9541 removed events despite the stated intention in the deprecation commit f5c2e3d to keep them. Not a plea to you, but to other mpv devs.

@guidocella
Copy link
Contributor Author

That removed 700 lines of code, it is not comparable to 1-line wrappers which cause no maintenance burden.

@po5
Copy link
Contributor

po5 commented Jan 5, 2024

5e0902c, 7020f0c (read: you're wrong) (says sfan)

@avih
Copy link
Member

avih commented Jan 5, 2024

These will probably never be removed

If this is what you think then don't deprecate. Deprecating means exactly one thing: it will get removed so users shouldn't depend on it.

This is hurting users for no good reason. Instead, just document better that it's identical to the properties. It costs us nothing to maintain these trivial wrappers, and it helps users which don't need to keep changing their scripts.

@guidocella
Copy link
Contributor Author

5e0902c, 7020f0c (read: you're wrong) (says sfan)

I don't understand what these have to do with getcwd and getpid, and I have no power to decide what mpv maintainers will remove either way.

If this is what you think then don't deprecate. Deprecating means exactly one thing: it will get removed so users shouldn't depend on it.

Per Wikipedia's definition https://en.wikipedia.org/wiki/Deprecation deprecation can mean that a feature is obsolete without removing it, as is the case with 40+ things listed in mpv's manual.

This is hurting users for no good reason. Instead, just document better that it's identical to the properties. It costs us nothing to maintain these trivial wrappers, and it helps users which don't need to keep changing their scripts.

No user is hurt because nobody wants to remove these functions.

I added a note that the functions will keep working regardless, but why only these 2 functions needed this note out of 40+ deprectaed things is unclear.

@N-R-K
Copy link
Contributor

N-R-K commented Jan 5, 2024

TBH I don't see any reason to "deprecate" them either. Just document it being identical to the property if you think it causes confusion.

@guidocella guidocella changed the title DOCS/lua: deprecate utils.getcwd() and utils.getpid() DOCS/lua: note that properties are preferred to getcwd and getpid Jan 5, 2024
DOCS/man/lua.rst Outdated
is returned.
is returned. This is equivalent to retrieving the ``working-directory``
property, which is preferred because it is available to clients other than
scripts.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay but this is not actually an argument.
If you are reading these docs you are in a Lua script. Whether other clients can retrieve the same stuff is irrelevant to you. You just want it from Lua.

Explain that these functions are equivalent to the respective properties
to prevent confusion like in
mpv-player#7975 (comment)
@sfan5
Copy link
Member

sfan5 commented Jan 14, 2024

In general exposing stuff via properties and not Lua-specific functions makes sense (so IPC clients are as powerful as scripts), but for these two I think it doesn't make sense to recommend properties.

What you want: the PID
utils.getpid: calls the libc
pid property: locking, dispatch to main thread, it calls the libc, mpv_node gets copied, structure unwrapping

there is no reason to have all this complexity.

tl;dr I vote to just close this PR

@guidocella guidocella closed this Mar 31, 2024
@guidocella guidocella deleted the deprecate-utils branch March 31, 2024 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants