[RFC] cleanup some deprecated options #14382
Conversation
|
Download the artifacts for this pull request: |
|
See https://github.com/mpv-player/mpv/blob/master/DOCS/compatibility.rst#renaming-or-removing-options. It specifically says that both of these mappings can remain in the code for a long time. Unless these option names are reused by newly added options, I don't see any benefit in removing these messages, especially when they don't incur any maintenance burden and removing them makes errors less user-friendly. |
|
I would definitely insist on removing at least |
|
I think it's OK to remove these aliases, but |
|
I personally think |
This difference worked when these options were never removed. These deprecated aliases simply stayed working so no special actions needed, and I don't think the users would take the deprecation warnings seriously enough. This whole option "cleanup" practice is a very new one started only 9 months ago (969c19c) which I think wasn't done with sufficient consideration, because these aliases suddenly stopped working without any information about replacements. If you want to continue this practice (which I personally find no practical benefit), I would at least keep them as |
What? Deprecated aliases inform users about replacements. Some of them in that patch series were spitting out warnings for several years. If someone doesn't bother to spend two seconds to update/change the thing that was removed, why is this our problem?
If nobody actually ever removes these things, then there's no point in ever deprecating them in the first place. |
As I already pointed out, both in Not to mention that several options in this PR only changed name 8 months ago, not even a year, and they're being removed without
The point of these things is to enable renaming options and removal of useless/broken code in a way that properly informs users while not leaving any real maintenance burden for the developers. The whole system was designed and used this way. Removing these information does nothing to improve code maintainability, since there are no code to maintain behind these flags. It only causes mpv to be less user friendly. The average user aren't chasing git master updates every day like you do, and they won't care until something actually breaks.
This does not preclude leaving |
Please consider that the average user does not update mpv every week. In some cases, users migrate multiple versions forward. For example, when updating Ubuntu LTS, you jump from 0.34.1 to 0.37, and if you skipped the previous LTS, you start from 0.32. Also, consider that there are guides and configs lying around in forums that are outdated. By not removing those aliases, we simply allow users to easily port those configs forward, for better or for worse.
I agree with @na-na-hi on this. Neither There are two types of option removal:
In both of these cases, |
I'm sorry but this is frankly ridiculous. The default message when you use |
Sure the option can be removed after some time and marked with EDIT: We could even have macro Same for normal deprecation, we could set a version which would trigger compiler warnings (or error) after 2 versions to remove the related code and mark such option |
|
Why is this suddenly a rule now? In #12436, I removed a bunch of crap including I'd totally get it if you guys said something like "option X is still used quite a bit in my opinion, let's hold off for a bit", but it basically sounds like a highly unreasonable amount of backwards compatibility which is not something mpv ever did. Especially not for dumb obscure options no one uses. |
The fact no one complained doesn't make this everyone agreed.
In fact what I'm saying mostly is that In wm4's words:
It is already removed, there is not much more we can do. And showing a meaningful message instead of EDIT: Also if we remove options after depeciation period, when and why we would even want to use EDIT2: I don't want to bikeshed this small thing forever, I just think @na-na-hi made a good point. But it is ultimately up to you, how to handle those things. |
I mean your own thumbs up is currently there so I presume you agreed. This PR is not really any different in principle. The options here are just not nearly as old as the ones there. As for |
Like I said, na-na-hi made a good point. I don't think it is strictly bad to remove those options, but now I see that we can do better without any additional cost in practice.
I feel like it would be inconsistent with the mpv cli options. Unknown options return error, removed should too. The only gain of No one reads warnings, until they are errors.
Of course not, I feel like it could be resolved easily with OPT_REMOVE_IN(0.35) which could print something like "removed in mpv v0.35" which is infinitely more useful and actually can be left until the end of time. But also can be removed completely if there is no information to say, except that it is no longer working. |
This is only there to discourage users from using that deprecated alias. But in practice, there is little reason to ever remove them unless the option it's aliased to is also removed, because a single line of
My point is that there is absolutely no reason to remove
There is no "unreasonable amount of backwards compatibility" here:
This is a small subset of cases (options now does nothing and has no replacement) compared to all of the other useful information removed. I think in this case only it's OK to omit In summary, is there any benefit with removing them? To make us "feel good"? IMO this reason isn't good enough. |
So what are you suggesting that instead of using .deprecation_message when removing an option we should just hard error when we remove something (see
If a user ignores a big fat warning for an extended period of time, I fail to see why this is our problem. It's doubtful such a person would even bother to read an error.
Why is this useful? Does anybody really need to know that mpv used to have some [insert random debugging option here] 20 versions later in the future?
Yes and eventually the endgoal is to make them unable to use it altogether. That's what deprecation means. Look if you really think
Useful? Why does any user need to know that
Yes. As stated earlier, I consider the existing deprecated alias Nobody uses All in all, I find this entire discussion completely ludicrous and it's clear we're not going to see eye to eye on this. If you were just telling me "hey dudemanguy, you're removing thing X too fast, it should stick around longer", then we can have a discussion. But instead apparently mpv should be changing its philosophy to keep atrocious deprecated options like It doesn't matter to me if having |
I think there is misunderstanding.
No it should be normal deprecation period.
User messages doesn't have to reflect perfectly the state of development. User messages should be understandable and clear for users. You don't explain rocket science to 5yo, you only say that rockets work.
Assumptions... last time we made an assumption about
Sorry, but I think you tunnel vision a bit. If we are strict about 2 releases policy for deprecation things removal, it is perfectly possibly that Billy, who doesn't know much, would have theirs config broken on Ubuntu LTS to LTS update. In such cases user can skip deprecation period, which is fine, we cannot sync to the release cycles of all the distros. But here comes If you think it is not good to annotate removed option, please remove the Note that @na-na-hi suggest to keep
We don't have to, but it is good to have all the arguments laid down. |
Again that's totally fine in some cases but as I've repeatedly said, there's a point where this is useless info that nobody needs. e.g. no user today is going to somehow type
"Warning: option --X was replaced with --Y and might be removed in the future." sounds 100% clear and understandable to me. If our policy is actually to never remove
Sure, I made a miscall on
I don't agree. The importance of options/properties is not uniform thus the deprecation process cannot be uniform. It is inherently a sliding scale of what is considered important and what is not. Well unless you want to use an ungodly long deprecation period for everything I suppose. Let's say that for some bizarre reason we decided to completely break On the other hand, let's take
This is just an argument for possibly a longer deprecation period for something which is totally fine. We can go on a case-by-case if anyone thinks it is important enough.
As stated before, |
It's already doing that: the default messages says "and might be removed in the future". I'm sure you know what the word "might" means. There was never a requirement that deprecated options must be removed after being deprecated for X time.
The option is already removed, so it's already broken. What you're doing here is exactly the opposite of "doing them a favor" by removing the message which informs the user about the replacement option to use, so that he can fix his config.
That's because the documentation of that option was very unclear. See #13804 (comment). This is the real reason why that option was misused. It has little to do with the naming of the option.
"changing"? Quite the opposite, since the only one who changes anything here is you. Lots of And guess what? Printing info about renamed options didn't even exist in the first place! It was ADDED in 7fb5df0 specifically because it's "more userfriendly." The whole point of the printing system is to improve user friendliness by objectively providing more information. Whether you personally think it's useful or not doesn't matter. Also notice that the renamed options didn't work in that commit, but a later change to the option system "unbroke" these renamed options and made them work transparently again.
I have explained again and again that removing useless options doesn't preclude providing
So no practical benefit. What does have a practical benefit though, is to remove this warning system altogether and print a generic message telling users to "RTFM" whenever they dare to use an option that doesn't exist. It would've achieved the same purpose with less amount of code to maintain. After all, who cares about user friendliness? /s
So what's the priority here? Removing useless code and inform users immediately with |
It also obviously doesn't mean "never". Don't be stupid.
Man now this is an amazing bad faith argument. Don't be ridiculous.
It's not a question. They were deprecated. 10 years is way more than enough time to remove them. I guess you think that
Nobody is contesting that the functionality can be useful. The only thing that is being argued here is that such options have had a sufficient deprecation period and the mapping has outlived its practical use.
And I have explained again and again that these options in question have already been providing deprecation messages for a long time.
Yes if you conveniently ignore the vast majority of my response to that question and give some wild bad faith arguments, sure.
Why is this so hard to understand? I've explained what I think |
|
Thank you for the work of every developer. I'd like to share some thoughts as an ordinary user. Take the example of --cache-dir mentioned earlier. According to the OP's suggestion, it would be deleted directly, and users would need to modify it themselves. However, when I checked the wiki and manual, I couldn't find any guidance on how to migrate. (In fact, with enough information, I could have known that I should set its sub-option, such as --icc-cache-dir. ) |
Thx! I get it. I forgot to read this docs, always search in manual, my fault |
|
Without getting into any discussion, I'd just share my point of view: Keeping OPT_REMOVED for all removed options is useful to the user and doesn't have any maintenance burden, therefore I support keeping OPT_REMOVED forever for all removed options, except for reasons I can't forsee right now which neccessitate deleting it completelty. That's not deprecation, because it's already past the deprecation preiod and already removed as a functional option, therefore it only served as a note to users "this doesn't exist anymore, do this or that instead", which is cheap for us and useful to the users. |
Why? I understand what you are saying, but why do you think longer deprecation time solves anything? We have already pretty long period, that certainly gives more than enough time for anyone to adjust to new changes if they want to. Deprecation period is to allow users to migrate, if they want. And by having uniform policy it would be clear when/how the option is removed/replaced. Even if we were to "break"
And why does it matter? Sure if it is not possible to have deprecation period. But otherwise why make specific exceptions of the rules, just because we assume it is not used? What does it help with, why cannot it go through normal period? To make us feel better that it was removed quicker? It is not like mpv will ever have changes that significant that they cannot be ported quickly. See I won't circle back on other points, because everything has been said already. I feel like we are overthinking it, in my opinion having consistent deprecation policy would not only help us avoid thinking about it, but also users to understand when each feature would disappear. EDIT:
The message says exactly what is says. option --X might be removed in the future. It is deprecated. It may be removed at any point, without additional notice. We are not saying it should be never removed. We are saying that until it works correctly with |
This is a real-time demonstration of what I meant by that deprecated alias being user hostile. It's not your fault. The name is extremely misleading and judging from this response you completely misunderstood what it did.
I've stated this earlier but I don't agree that it is forever useful and given enough time it would be noise or possibly even confusing if someone happens to type in some ancient obscure option from a decade ago. We've removed plenty of options before with no special message left behind. Apparently I am in the minority on this or something so maybe someone make PR documenting the proposed policy change.
It's sounds like our divergence is that you think 2 full releases is sufficient for everything. I don't. There's no reason we would ever do this but if we completely break
Yeah I think we don't disagree on this part as much as I initially thought. Currently, the only policy we have is basically "important stuff should be deprecated for a while; unimportant stuff can be removed whenever you feel like it". So to bring up the Anyway, if you want to change it to where everything must have at least some minimum deprecation period. I would not be opposed to that. My only pushback is that the difference between "important" and "unimportant" stuff is still meaningful and "important" things would likely need an even longer deprecation period than whatever the minimum is. So in my view, that's not technically a uniform policy however changes like that should be extremely rare at this stage. There should virtually be no reason why any of us would need to change any of the well-known highly used options in a non-compatible way.
That has never been my interpretation. If we are actually saying this, documentation should be updated. |
I'm just saying that they weren't normally removed in practice because there is little reason to do so. I also already pointed out some reasons why removing them is beneficial. But I never said that you can't ever remove them. You can remove them with
It's the same "feel good" argument here. You only want to apply Sure, you can just tell them to "RTFM", but then what's the point of this warning system in the first place? Just delete it already. It's not existing in a meaningful way at the current state, and requires continuous effort to "clean up" manually. Using it in the intended way or deleting the system completely would relieve us of this manual labor. I won't elaborate on the rest, which is basically "I know what's good and what's not for our users, and if some users don't use mpv in the way we want, well, fuck'em."
It's already removed from the documentation. No new users are going to use it. The hostility is no more. You're just conveniently ignoring the fact that the existing users don't know how to migrate now because you're unwilling to leave an |
Exactly and that's why As @avih said:
Dude... no one does that. I mean really. There are two kinds of users/script integrators, the ones that are active and will manage to fix it in 2 releases (which is 1-2 years) and the others that are not active or don't care and will fix it only after it becomes hard error, or never. Side note, removal should be done on the beginning of release period to give most amount of time before stable release for latecomers. I would consider longer deprecation period only if we break completely libmpv api, requiring significant development effort to integrate new version. Or if we deprecate the mpv custom shader hook in favor of new system. Or deprecate I agree some deprecations can be left for a long time, but I focused on
We have it in the documentation: Lines 55 to 59 in bab9b2c I agree, that some things may not be important, but still it doesn't harm us to just wait a bit. Doesn't matter if we remove now or later, if the option is not used anyway.
The whole point of |
|
It's not me who got the wrong option. I just told him to check the doc. |
You just complained that I removed 10 year old deprecated aliases, but OK. Formalize this. In what circumstances is it OK to remove them? 15 years? Only if the thing that it is aliasing also disappears? With regards to OPT_REMOVED, is this required when removing any option or only in certain circumstances? How long should an OPT_REMOVED be in the codebase? 10 years? Forever? If I'm in a retirement home and mpv is still maintained, should it still have an OPT_REMOVED informing users that --gamut-clipping was replaced by --gamut-mapping-mode=desaturate 5 decades ago (assume for the sake of this argument that --gamut-mapping-mode is still an option)? Clearly you feel very strongly about this so formalize this in the documentation and make a PR.
That user expressed that view based on a complete misunderstanding of an option (explained later). I fail to see how this backs you up in anyway. You used arbitrary as a pejorative here, but the process is fundamentally already arbitrary. Developers make judgement calls on deciding what should be removed, renamed, etc. There is no hard rule for this and can be for a variety of reasons. Your view that OPT_REMOVED should never be removed is just as arbitrary as my view that after enough time passes, it becomes useless code. It is made based upon individual judgements/opinions on what software development practices should be.
This is what I would consider yet another argument made in bad faith. What you tacitly claim is "the intended way" is nothing more than your personal opinion. It is perfectly fine to think I am an idiot that uses the system in a poor way and so on. But you cannot claim to know what the true intent is. The only person that can do that is the one who actually wrote it which is not any of us in this thread. As for the "manual labor" point, I would file that in a "not an argument" category. mpv is maintained software, so developers will have to crack open some files and change things. It's how it is. If you wish to eliminate the "manual labor" of maintaining options/properties/apis, etc., then the only conclusion is to never remove any deprecations (or maybe never deprecate in the first place but that's quite unfeasible) and leave these things in the codebase forever. If that is the policy, it needs to be documented. Developers will naturally remove old deprecated things to clean up codebases.
An And also I don't know why you keep acting like there was no warning that tells users how to migrate. It was just deemed no longer needed and thus deleted. If you think this guidance should last longer/indefinitely/etc. in some form, please elaborate on what your proposed policy like I said earlier.
Very nice of you to go into baseless personal attacks and completely misrepresent my arguments.
This is completely speculative, but I think this is optimistic. If we completely break something major that is used all the time, I would expect the pain to last many years. But I suppose there's no way to know without trying, not that we should. I can't think offhand of something that we did in the past that would really fit this category. But maybe you're right and it would all be fixed pretty fast. Who knows.
Right but section there clearly says "Important/often used parts". That's not everything. Right below, it says "Less useful parts can be broken immediately". P.S. thanks for the actual PR feedback. I appreciate it. I have no intention of arguing forever with you. |
Dudemanguy
force-pushed
the
deprecation-cleanup
branch
from
5c268d4
to
2cacc7c
Compare
| remove deprecated `--cache-dir` option alias | ||
| remove deprecated `--cache-unlink-files` option alias | ||
| remove deprecated `--demuxer-cue-codepage` option alias | ||
| remove deprecated `--fps` option alias | ||
| remove deprecated `--override-display-fps` option alias | ||
| remove deprecated `--sub-ass-force-style` option alias | ||
| remove deprecated `--cdrom-device` option alias | ||
| remove deprecated `--play-dir` option alias | ||
| remove deprecated `--sub-forced-only` option alias | ||
| remove deprecated `--vo-sixel-exit-clear` option alias | ||
| remove deprecated `--cdda-toc-bias` option | ||
| remove deprecated `--drm-atomic` option |
There was a problem hiding this comment.
Some of them were renamed at 0.37. Based on the negligible benefit I consider removing renamed aliases must follow the 2 full release deprecation period. #12436 followed that: 0.37 was in development, and renames from 0.35 weren't removed.
These aliases can be removed after 0.39 is released.
There was a problem hiding this comment.
Are we counting differently? All of these were options were deprecated in 0.37, no? That's 1 release. 0.38 is 2 releases. So the upcoming release, 0.39 is fair game by that rule.
There was a problem hiding this comment.
I want to make sure that the removal is synchronized with interface-changes.rst, which is now only updated at release. Under your interpretation, there will be a period that the removed options have no consumable documentations before the next release, which is the precondition I have for #14396.
I think the best thing to do here is make cleanup of deprecated aliases as a part of the release routine, not here.
Punishing the user for using
You shouldn't expect complaints to come in overnight. Despite what the code comments said, the |
Almostly every release will change/remove/rename some options/parameters. 'Breaking' happens all the time. |
It was actually originally
I'm sorry that happened, but it's not really to comparable to anything here or in the other PR. No |
These were all deprecated in mpv 0.37.0 or earlier and are not considered common enough options to warrant keeping the deprecated mapping longer. Since demux_cue had only a single option in it, the entire option substract is removed. This can be readded later if someone wants to introduce a specific option to it again.
Most of these are pretty obscure things that were replaced a long time ago. The special messages they were printing are also not really useful at all so just remove them.
In both cases, setting these options did nothing other than give you a warning that they may be removed in the future. Remove them now.
Dudemanguy
force-pushed
the
deprecation-cleanup
branch
from
2cacc7c
to
8daf406
Compare
|
I left the deprecated aliases for |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
Felt like doing some janitor work again. All of these were from mpv 0.37 or earlier, and I see no reason to continue leaving these deprecated/useless options in the codebase for years. Let me know if you think if any of these are too soon. I also didn't document the removal of the
OPT_REMOVEDones because mpv fails opening if you use any one of those so it's not really a terribly relevant interface change imo. Just the removal message goes away.Didn't touch the
--gamma-autoor--gamma-factorstuff since we already have a PR for that (#14369).