Simplify engine version requirement method #5
Conversation
There was a problem hiding this comment.
Added a few notes inline. On your final comment:
It's also possible, that the previously used getVersion method could be removed. For display purposes there is getVersionString, but if requirement checking is not in the scripts, getting the numbers separately may not be necessary.
I actually think it does make sense to keep them, not only for more flexible checks (as one of my comments inline), but also to allow developers to use possibly slightly different APIs to do the same thing across different versions if they're aware of how they differ.
src/scripts.c
Outdated
| int major = luaL_checknumber(s, 1); | ||
| if(major != KIAVC_VERSION_MAJOR) { | ||
| SDL_LogError(SDL_LOG_CATEGORY_APPLICATION, "[Lua] KIAVC major version number incompatible: %d (expected %d)\n", KIAVC_VERSION_MAJOR, major); | ||
| kiavc_cb->quit(); |
There was a problem hiding this comment.
Indentation seems wrong in this checks and the ones below? Looks like you're using spaces instead of tabs (which I'd rather we use in this project, since it's what I'm used to).
There was a problem hiding this comment.
Sorry, I'm used to the opposite, so I didn't notice, but it's fixed now.
| return 0; | ||
| } | ||
| int minor = luaL_checknumber(s, 2); | ||
| if(minor > KIAVC_VERSION_MINOR) { |
There was a problem hiding this comment.
I get the SemVer reference you made, but I think it can cause ambiguity if someone is interested in a specific version. Maybe there could be different checks, where one checks if the version is exactly that, and another one if it is at least that (which could loosen the check on the major version as well). We could even just keep the "exact version" check, and leave more flexible checks to the application (using the getters for the three version numbers that exist).
There was a problem hiding this comment.
I wouldn't want to loosen the major version in a minimum requirement, because that number is meant to signal breaking API changes that are not backwards-compatible.
My proposed solution is to have two formats, like you said, one being the exact one (now requireEngineVersion), but the other one being the SemVer one, which I renamed to requireEngineCompatible. See commits.
This change moves the checking of the engine version into the engine code, simplifying the method call needed in the game Lua file.
Behavior is slightly different: major version quits on any difference, minor and patch only quit if the required version is larger than the available one. This is consistent with SemVer, but a matter of decision.
The code itself definitely needs to be checked. I believe I added the change in all required places and tested it running on my machine, I'm not yet familiar enough with the codebase.
A possible issue is that the error log entry is displayed after script loading, but the init INFO entries are all still displayed before the game exits, obfuscating the source of the error further up in the log. On the other hand, the current version fails with an INFO on "Unsupported KIAVC engine version", then two CRITICAL-s: screen resolution and engine init. While this is obviously shorter and clearer, the screen resolution line could still be misleading.
It's also possible, that the previously used getVersion method could be removed. For display purposes there is getVersionString, but if requirement checking is not in the scripts, getting the numbers separately may not be necessary.