[BC] Allow more changes when the class/method is final #7611
Conversation
contributing/code/bc.rst
Outdated
| Change argument type No | ||
| Change return type No | ||
| Add default value to an argument No [7]_ | ||
| Remove default value of an argument No [7]_ |
There was a problem hiding this comment.
This change looks wrong to me. Removing a default argument influences not only child classes, but also changes the way a consumer can call the method.
There was a problem hiding this comment.
👍, I'll change that asap (editing the file on my phone changed the spaces).
contributing/code/bc.rst
Outdated
| Add type hint to an argument No [7]_ | ||
| Remove type hint of an argument No [7]_ | ||
| Change argument type No [7]_ | ||
| Change return type No [7]_ |
There was a problem hiding this comment.
The changes above would IMO need some clarification (not all changes are allowed, but only those that are compatible with how the code was used before, i.e. widen a type-hint for example).
There was a problem hiding this comment.
Indeed you're right, do you think widening a type hint is worth being explained or should we just not have a note here?
| Move to parent class Yes | ||
| Add argument without a default value No | ||
| Add argument with a default value No | ||
| Add argument without a default value No [7]_ |
There was a problem hiding this comment.
I updated the notes to separate final classes and final methods.
A protected method in a final cla
8000
ss is like a private method so this change is allowed, but it is not for a final method, so it should be ok now.
Thanks for your comments!
| Reduce visibility No | ||
| Remove protected method No [7]_ | ||
| Change name No [7]_ | ||
| Reduce visibility No [7]_ |
There was a problem hiding this comment.
[8] applies to all three above too, doesn't it?
There was a problem hiding this comment.
No because having a final protected method doesn't prevent people extending the class from calling the method.
contributing/code/bc.rst
Outdated
| Change return type No | ||
| Add default value to an argument No [7]_ [8]_ | ||
| Remove default value of an argument No [7]_ | ||
| Add type hint to an argument No [7]_ |
There was a problem hiding this comment.
This can be allowed if the method is final because if you cannot extend the method, the added typehint (that is compatible of course) won't do harm.
There was a problem hiding this comment.
What do you mean by is compatible? We can't allow this change in all cases so imo we should not change that.
There was a problem hiding this comment.
eg if the code throws after an instanceof check, then it's ok to add the type hint to me also
string also when scalar is checked
may need a few words thought to make clear what's possible (would adding "as long as it preserves BC" be enough?)
There was a problem hiding this comment.
I updated the notes to:
Allowed if the class is final. Classes that received the ``@final``
annotation after their first release are considered final in their
next major version.
Changing an argument type is only possible with a parent type.
Changing a return type is only possible with a child type.
Is it ok for you?
contributing/code/bc.rst
Outdated
| Remove argument Yes [3]_ | ||
| Add default value to an argument No | ||
| Add default value to an argument No [7]_ [8]_ | ||
| Remove default value of an argument No | ||
| Add type hint to an argument No |
There was a problem hiding this comment.
same here, should be possible when final
There was a problem hiding this comment.
I don't think so, if someone calls the method he expects that everything will work as the argument if there is no type hint. If you add a type hint then not everything will work anymore. I think this change needs a bc layer.
There was a problem hiding this comment.
The changes are always under the premise that the behavior is BC. This also counts for the things like "Removing an argument"
There was a problem hiding this comment.
Ok, that's a good point. Then we should also allow Change argument type and Change return type for final methods, right?
There was a problem hiding this comment.
changes should only be allowed for parent types, not siblings nor children
contributing/code/bc.rst
Outdated
| @@ -277,6 +277,12 @@ Change value of a constant Yes [1]_ [5]_ | |||
| Additionally, if a constant will likely be used in objects that are | |||
| serialized, the value of a constant should not be changed. | |||
|
|
|||
| .. [6] Allowed using the ``@final`` annotation. | |||
|
|
|||
| .. [7] Allowed if the class is final. | |||
There was a problem hiding this comment.
or when the class has the @final annotation since the previous major version (same below)
There was a problem hiding this comment.
I guess we can call that being final, don't you think? 😛
There was a problem hiding this comment.
what I think doesn't matter: what the next reader will understand does :)
There was a problem hiding this comment.
In fact I don't see how we could clearly explain that the @final annotation means that the class/method is final but only if it is present since the previous major version, I think it is clear enough as is.
There was a problem hiding this comment.
this doc is here do make things explicit, I think even if it's clear for us, I'm sure it's not for someone not used to our processes. since we always add the version were it was made final, I don't see the issue (except the "wording" one of course, but that's solvable :)
|
Thanks @GuilhemN! |
…uilhemN) This PR was squashed before being merged into the 2.7 branch (closes #7611). Discussion ---------- [BC] Allow more changes when the class/method is final Fixes #7597 Commits ------- f5b8dd4 Improve notes 936200c Update 4a3fbed Fix comments a5f6d24 [BC] Allow more changes when the class/method is final
Fixes #7597