Change old command removals to use RemovedCommand #369
Conversation
There was a problem hiding this comment.
Looks good overall. In some places (but not all) you removed the previous deprecation documentation. Was this on purpose? I think it should be left alone so that users can see the progression from active to deprecated to removed. Also if they are using the previous version it may only be deprecated for them.
|
Let me explain about the lilo command classes, it's a bit tangled, what with the definitions evolving over time :-) Thing is, the FC3 lilo command claimed it is deprecated in the next version - FC4. That doesn't happen elsewhere, commands always note only changes in the same version. So that seemed wrong. And then FC4 didn't have a class for this command in handler so it wasn't "deprecated", it was really "removed", at least in the sense of how things work now. So that makes it a pretty unique command that has exists in only one version, and claims to be deprecated while being removed. (Does it?) So the choice I mention in first comment was: I went with what the actual code did, instead of the doc string. I got rid of the incorrect mention of deprecation, and made the command properly removed. That was also the least amount of change. If I were to instead make the code fit the docs, and do so in the context of the changes applying to today's code ran today, I would instead drop the docstring on the FC3 class, create a FC4 class that is properly deprecated, and a FC5 one that is removed. That's the alternative I immediately see. So, what are your opinions, ideas...? I'm happy to change this, just not sure what I should do. |
Ah! I missed that detail, now it makes sense. Thanks! |
|
Thank you, too! |
This should make all removals conform to The New Way™.
Most of this is straightforward, but the lilo-related commits needed making choices about how to classify deprecated/removed.