-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Rewrite pkgmgr and contentdb in C++ #14763
base: master
Are you sure you want to change the base?
Conversation
Note that per our coding style, functions that are not methods should use snake_case naming. |
@JosiahWI The usage of snake case in the lua bound methods seems consistent, so there's nothing wrong there. |
src/content/content.h
Outdated
@@ -50,6 +55,10 @@ struct ContentSpec | |||
std::string textdomain; | |||
}; | |||
|
|||
std::string contentTypeToString(ContentType &t); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be content_type_to_string
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look 2 lines below that function. Those were in camelCase as well and I didn't write those. What do you want me to do there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've rewritten that function's name but left the other 2 alone. If you want me to change those, do tell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are methods as opposed to non-method functions and therefore are named in the correct format. I don't know why we name methods and non-methods differently - that's just the way it is in our style guide. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They aren't methods, they are just wrapped in a namespace. They are regular functions. They really should be camelCase
static bool isValidModname(const std::string& str); | ||
static std::string getContentdbId(const ContentSpec& content); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I see now, because these are static they should be snake_case.
static void Initialize(lua_State *L); | ||
static void InitializeAsync(lua_State *L, int top); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
Rewrites pkgmgr and contentdb functions in C++, as well as tests. Will close #12295.
The intention of this PR is the start of adding a CLI and TUI interface for mod management, updating, configuration, which I plan to do later in a separate PR. Of course, that will close in the future #7358 (along with an additional ncurses TUI) and some other features.
It was discussed in #minetest-dev if it was worth just loading a Lua runtime on the CLI. However, since tests weren't written yet, this rewrite shouldn't be hard, and an issue was already requesting it, I figured I would tackle it. It will also add a convenient C++ API for these functions without a Lua dependency.
To do
ATM I got the base done here and one simple function ported to wrap my head around the minetest-lua codebase. This is a draft, please do not merge.
How to test
Coming soon in theaters June 2024