Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change _VERSION to display version of luau in use #176

Closed
wants to merge 1 commit into from
Closed

Change _VERSION to display version of luau in use #176

wants to merge 1 commit into from

Conversation

Baileyeatspizza
Copy link
Contributor

this is just the commit for the issue I made with the same name #175

please keep in mind this is my first time using C++ so it may not work but from running make test it seems fine
Screenshot 2021-11-08 at 10 47 40

I mean it works which I didn't expect but yup

in the future it may be beneficial / convenient to store the version in like a Version.txt file but I obviously have no idea how to do that

this is such a minor change I don't expect it to cause any issues as its backwards compatible with any code expecting it to return Luau

and even better it follows Lua's format for _VERSION
Screenshot 2021-11-08 at 10 51 10

@MathematicalDessert
Copy link
Contributor

MathematicalDessert commented Nov 8, 2021

Shouldn't this be a variable somewhere else? You could use a group of define directives or a group of constexprs to split version into Major, Minor, and Patch.

constexpr auto LUAU_VERSION_MAJOR = 0
constexpr auto LUAU_VERSION_MINOR = 503
constexpr auto LUAU_VERSION_PATCH = 0

The above is an example where patch is included.

The version string would then be a concatenation of these variables with Luau prepended.

@Baileyeatspizza
Copy link
Contributor Author

Baileyeatspizza commented Nov 8, 2021

I don't really see the need to separate it into multiple variables just changing _VERSION to return all would basically solve that and people can parse what's returned by _VERSION to separate it into Major - Minor - Patch

_VERSION as the name implies should just be the version its running

and it'd be backwards compatible with Lua because Lua returns _VERSION as "Lua Major.Minor"

the patch is just an additional thing that may be nice but may cause issues trying to parse the version

@MathematicalDessert
Copy link
Contributor

MathematicalDessert commented Nov 8, 2021

Right, I added patch as an example. Even if they aren't separated into distinct variables, I don't think it makes sense to edit that string there all the time. It should be somewhere else, no?

Lua already does something similar: https://github.com/lua/lua/blob/master/lua.h

@Baileyeatspizza
Copy link
Contributor Author

Baileyeatspizza commented Nov 8, 2021

well I put it in the pull request that ideally it should be put in a version.txt file

but yeah that's my first time ever using C++ so I wasn't gonna attempt doing that myself

version.txt would also be beneficial as you wouldn't have to dig into the file system / code just to get the version in use

@@ -449,7 +449,8 @@ LUALIB_API int luaopen_base(lua_State* L)

/* open lib into global table */
luaL_register(L, "_G", base_funcs);
lua_pushliteral(L, "Luau");
lua_pushliteral(L, "Luau 0.503"); /* set _VERSION to display the version of Luau currently in use */
Copy link
Contributor

@MathematicalDessert MathematicalDessert Nov 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you change this to:

lua_pushliteral(L, LUAU_VERSION);

and add a string literal in lua.h?

e.g.

// classic style
#define LUAU_VERSION_MAJOR "0"
#define LUAU_VERSION_MINOR "503"

#define LUAU_VERSION "Luau " LUAU_VERSION_MAJOR "." LUAU_VERSION_MINOR

// OR modern style
constexpr auto LUAU_VERSION_MAJOR = "0";
constexpr auto LUAU_VERSION_MINOR = "503";

constexpr LUAU_VERSION = "Luau " + LUAU_VERSION_MAJOR + "." + LUAU_VERSION_MINOR;

Copy link
Contributor Author

@Baileyeatspizza Baileyeatspizza Nov 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I suppose but I'd still prefer if a separate file (preferably .txt) could be used to track the version for ease of access when updating the version. To make it easier for new people to view the version without having to dig through the C++ code or start running luau. But that may be just another pull request later instead.

Edit: appears you fixed the issue ignore what I originally put here

Copy link
Contributor Author

@Baileyeatspizza Baileyeatspizza Nov 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I just realised that a version.txt file may not work properly with the binaries unless the value is saved during compiling.

so your suggestion may be the best course of action unless the people who maintain luau want it to just be a single string instead of that string concatenation etc.

either way a variable somewhere else in the code is probably better then writing it in the function

@JohnnyMorganz
Copy link
Contributor

this is such a minor change I don't expect it to cause any issues as its backwards compatible with any code expecting it to return Luau

Note: this is not backwards compatible with the following code:

if _VERSION == "Luau" then
    print("using luau")
end

@Baileyeatspizza
Copy link
Contributor Author

Baileyeatspizza commented Nov 8, 2021

this is such a minor change I don't expect it to cause any issues as its backwards compatible with any code expecting it to return Luau

Note: this is not backwards compatible with the following code:

if _VERSION == "Luau" then
    print("using luau")
end

code like that really shouldn't be in use anyway as Version can change at any point without warning

here's a better method:

if string.match(_VERSION, "Luau") ~= nil then
	print("Running luau")
end

here's another better method

if string.sub(_VERSION, 1, 4) == "Luau" then
	print("Running luau")
end

Screenshot 2021-11-08 at 14 30 43

but it is backwards compatible with any Lua code to get the versions Major / Minor

however I do see your point it would break those very specific lines of code but in doing so it allows it to be more similar to Lua's format of Major / Minor "Lua x.x"

also because luau is brand new there shouldn't be code out there to directly compare if _VERSION = "Luau" and if there is its most likely going to be maintained since luau is new. So while there is a risk I highly doubt it'll break anybodys code and if it does well its a simple fix

@zeux zeux closed this Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants