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

A potential solution to luascript.cpp length #3504

Open
Zbizu opened this issue Jul 21, 2021 · 6 comments · May be fixed by #4207
Open

A potential solution to luascript.cpp length #3504

Zbizu opened this issue Jul 21, 2021 · 6 comments · May be fixed by #4207

Comments

@Zbizu
Copy link
Contributor

Zbizu commented Jul 21, 2021

This doesn't quite fit to any of the issue templates so I'll try to improvise.

The problem

luascript.cpp is too long and is causing some (CI?) tools to fail

Suggested solution

keeping lines such as this one: registerClass("Creature", "", LuaScriptInterface::luaCreatureCreate); to maintain the self-documenting style of code

move functions such as this one: static int luaCreatureCreate(lua_State* L); to a new file in a separate directory to reduce luascript.cpp size

Example for Creature class

in luascript.cpp:
#include luascript/creature.h
and the creature cpp/h would be located in:

  • src/luascript/creature.cpp
  • src/luascript/creature.h
@Kamenuvol
Copy link
Contributor

Kamenuvol commented Jul 21, 2021

I prefer like in otclient, just 'redirection' to c++ functions of given class

@yamaken93
Copy link
Member

I prefer like in otclient, just 'redirection' to c++ functions of given class

different kinda of lua bridge tho.

@nekiro
Copy link
Member

nekiro commented Jul 22, 2021

Isn't a bad idea imo, but it will increase compilation time significantly

@DSpeichert
Copy link
Member

luascript.cpp is too long and is causing some (CI?) tools to fail

Which ones fail now?

@Zbizu
Copy link
Contributor Author

Zbizu commented Aug 3, 2021

#2169

I had this in mind.
Feel free to close if you consider it resolved.

@DSpeichert
Copy link
Member

Maybe we should keep the discussion in the #2169, since they both bring up ways to do so.

Since now we don't have an active issue with the length of that file, current CI works fine (and we stopped using Travis CI), I don't think it's very high priority. There are many changes/rewrites of things in-flight as-is :)

@ranisalt ranisalt linked a pull request Sep 8, 2022 that will close this issue
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants