Document how to handle compatibility breakages #9158
Conversation
AThousandShips
added
enhancement
content:new page
There was a problem hiding this comment.
Thanks a lot! Sounds good so far, my feedback is very nitpicky 😅
Regarding an example, maybe we should use a real one that occurred? Then we don't need to make up things. It probably illustrates the principle better than an abstract one with "arg1", "arg2" etc.
|
Thanks for starting on this! I look forward to linking to it in the future :-) |
AThousandShips
force-pushed
the
compat_doc
branch
2 times, most recently
from
63f7dbf
to
f3e28b0
Compare
|
Added a concrete example, will go back over the style review as well, and the wording in the new example code isn't perfect, any feedback would be appreciated there |
AThousandShips
force-pushed
the
compat_doc
branch
from
f3e28b0
to
dcb5533
Compare
AThousandShips
force-pushed
the
compat_doc
branch
3 times, most recently
from
c14e505
to
04ed066
Compare
| So you've added a new parameter to a method, changed the return type, | ||
| changed the type of a parameter, or changed its default value, | ||
| and now the automated testing is complaining about compatibility breakages? | ||
|
|
||
| Not to worry, there's a system to handle just that! | ||
|
|
||
| Compatibility is handled in a few different ways. In GDScript this is simple, | ||
| new arguments are usually given default values when added, so existing scripts | ||
| won't break in such cases. | ||
|
|
||
| However in other cases, like GDExtension and C#, the exact way the method is | ||
| declared matters and must be matched exactly. This is handled by creating | ||
| compatibility methods that then get mapped to the new versions of these | ||
| methods. All of this happens *almost* automatically, there are just a few | ||
| things you need to do. |
There was a problem hiding this comment.
I feel we need to call out the different types of "compatibility breakages" more explicitly:
- For GDScript, adding a new parameter at the end of a method prototype with a default value does not break compatibility, but it does for GDExtension and C#, as described here. This is the only "compat breakage" that is ok to do in general.
- All other compatibility breakages break GDScript even if compat methods are added, so they can only be done exceptionally with approval for the release management team.
Not sure how to clarify this, with the introduction already being a bit long. I think my main issue is that the light hearted "no worry, there's a system that lets you break compat" may give the wrong impression about what kind of changes are allowed by default.
There was a problem hiding this comment.
Agreed will see what I can come up with, maybe we can cut this down and make a separate page discussing the general topic of compatibility focused on the abstract instead of the specific
| ClassDB::bind_compatibility_method(D_METHOD("get_id_path", "from_id", "to_id"), &AStarGrid2D::_get_id_path_bind_compat_88047); | ||
| ClassDB::bind_compatibility_method(D_METHOD("get_point_path", "from_id", "to_id"), &AStarGrid2D::_get_point_path_bind_compat_88047); |
There was a problem hiding this comment.
Would be good to have an example that uses DEFVAL(), or at least this needs to be called out that if the previous binding of a method had default values assigned, the compatibility method should have them too.
There was a problem hiding this comment.
Will see if I can find another example that has that to replace this with to make the example more complete
|
Thank you for the review! Will try to update later today Edit: Cleaned up and fixed the style details, will think about the intro section and update that later today and see if I can find a better example that has old default values and emphasize that the default values of the new method and the old method should be carried over, will see what to add there |
AThousandShips
force-pushed
the
compat_doc
branch
from
04ed066
to
b03da72
Compare
| Unless the change in compatibility is complex the compatibility method should simply call the normal method directly, instead of duplicating | ||
| the method, making sure to match the default arguments of the bind. |
There was a problem hiding this comment.
Added this to emphasize this part, might be worded better
|
Will look at the intro section and a more complex example case soon |
AThousandShips
force-pushed
the
compat_doc
branch
2 times, most recently
from
3724cb3
to
0934ef2
Compare
|
There, reduced the intro section and made it clearer that breakgages should be avoided, and added a few |
AThousandShips
force-pushed
the
compat_doc
branch
from
0934ef2
to
5b9afaa
Compare
AThousandShips
force-pushed
the
compat_doc
branch
from
5b9afaa
to
3fafd46
Compare
|
I've been planning to do it for a long time and it'll save myself work too as I've largely been the one to instruct how to do this on PRs 😅 |
|
Thanks, that's a great piece of documentation for contributors 🎉 |
|
Thank you! I'll experiment with some improvements to some of the wording later and we can discuss how to phrase the policy as well here |
Basic instructions and examples for using this feature