`GetNumMounts` has been changed to `getNumMounts`, breaking PWCU

[stephengtuggy]

PascalCase to camelCase, in other words.

[BenjamenMeyer]

NOTE: Changes like these that effect the Game Assets (VS:UtCS, PWCU, etc) need to wait until after the Game Assets are able to detect the engine version through the API. Until then we need to maintain a stable interface there.

[stephengtuggy]

@BenjamenMeyer Agreed.

Note that this particular change may actually be a good one to make in the long run. It's just that we're not ready to make it yet.

[ermo]

Unpopular opinion:

I'd actually prefer the new code as the VS unit code is all over the map in terms of consistency and this "bug" is not as much a bug as it is as about someone actually taking the time to take on the chore of making the exported Python method names consistent so you don't need to guess.

I would keep the changes and make it a priority to ensure that both PWCU and VS:UtCS work with the renamed functions as a baseline, since it also forces thorough dataset py3 testing, which is absolutely necessary in any case given that the present milestone is about dropping py2 and moving exclusively to py3 anyway.

</€ 0.02>

[stephengtuggy]

@ermo I hear what you're saying. And I, too, appreciate @royfalk 's attention to detail here. However, one of the cardinal rules of APIs is not to break backwards compatibility -- at least, not without incrementing the version number.

In fact, SemVer would say that one should increment the major version number whenever making a breaking change. And that if you're worried about breaking changes, then that's probably a good sign that your software should be at revision 1.0.0, if not higher.

[ermo]

Reverting the change because of some theoretical musings about API seems a bit odd to me when this milestone is in fact about a change that drops compatibility with a major version of Python (if that's not an API change, what is?)

Since it's already an API change, why not also take the opportunity to clean up the (now) python3(-only) code too while we're at it and benefit from the extra consistency without making a big fuss about it?

The C++ code is already there and the code needs to be audited for py3 things anyway, so this is as simple as ensuring that all C++ module calls are camelCase in the python3 code and that's the end of that task.

[stephengtuggy]

@ermo if it's just this one getNumMounts change that we're considering, then yes, you do make a valid point -- it's not that big of a deal one way or the other. It sounds like we have more API changes we want to make though. It also sounds like we are going to go ahead with a v1.0.0 release after v0.8.x. (See gitter discussion for more details.)

[BenjamenMeyer]

0.9.x got the Engine API stuff in #578, and I just backported it in #614 to 0.8.x.

The VS:UtCS assets are also able to detect it now. Not sure off hand if that got ported over to PWCU, but we have a pattern that can be utilized in the game assets now too.

[stephengtuggy]

Maybe the best solution here is simply to have the engine supply both versions of the function. getNumMounts and GetNumMounts. And export them both to Python.

[BenjamenMeyer]

@stephengtuggy that may be a good thing to do for migrations; the one version could emit a lot of please change to <new name>. In Python directly, that'd be done with the @deprecated decorator (https://pypi.org/project/Deprecated/).

Couple possible solutions:

• Export one one the Python side (the old version) and the other on the C++ side; then use our own version of the @deprecated decorator (so we didn't have to worry about imports) to log where the one is called so we/folks can clean it up.
• Export both on the C side, but then old version add a log to record that it's been deprecated. We could probably use a C Macro to efficiently generate the log message with the function name so that we have something re-usable (https://stackoverflow.com/questions/15305310/predefined-macros-for-function-name-func; https://www.boost.org/doc/libs/1_58_0/boost/current_function.hpp)

I wouldn't want to be maintaining both versions indefinitely; but having a deprecation path (and we probably should define a general policy for it too) would be a good thing.

[ermo]

What's the status on this?

Are both supported in the 0.8.x engine branch?

EDIT: I've changed it to the getNumMounts VS Engine API in pwcu as I'm planning to tag a 0.8.0 version of that which is tested to work with VS 0.8.0.

[stephengtuggy]

@ermo Cool. Glad to see the progress on PWCU.

[BenjamenMeyer]

@ermo 0.8.x should have the original code; 0.9.x is allowed to change it if we also increment the engine version; it's one of the reasons we introduced the engine version API so that we could start making breaking changes and the assets would be able to know if they'd be compatible.

[stephengtuggy]

Theoretically, nothing should have changed in the API as of 0.8.x; however, in practice, I think getNumMounts did change. The corresponding change you made, @ermo , was a good idea, I think. PWCU and version 0.8.x of the Vega Strike Game Engine should now be fully compatible.