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

Added EmmyLua export #19763

Merged
merged 2 commits into from May 25, 2022
Merged

Added EmmyLua export #19763

merged 2 commits into from May 25, 2022

Conversation

Mailaender
Copy link
Member

This was used to generate https://github.com/Mailaender/vscode-openra-lua/blob/main/api/library/OpenRA.lua which is an @EmmyLua compatible API annotation compatible with many IDEs.

Copy link
Member

@pchote pchote left a comment

Choose a reason for hiding this comment

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

Time to drop the ZeroBrane export too?

@Mailaender
Copy link
Member Author

Mailaender commented Oct 20, 2021

Time to drop the ZeroBrane export too?

Why drop? ZeroBrane does not support @EmmyLua. The existing integration still works. Just upstream does not accept my patches pkulchenko/ZeroBranePackage#86 which isn't really motivating.

@penev92
Copy link
Member

penev92 commented Oct 20, 2021

That ZeroBrane repository seems quite dead, so that might be why they aren't accepting your PR.

Copy link
Member

@penev92 penev92 left a comment

Choose a reason for hiding this comment

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

Some things in your example Lua file seem to be added manually - like the leading @alias, @class initTable and @fields. Those should be added to the C# code that generates the file.

Another thing that I'd like to see is we group everything inside their respective tables instead of defining an empty table and then the next 400 lines of code populate it before switching to the next table on the 401st line. I know this is auto-generated code and is not supposed to be user-facing, but increased readability is always a good idea IMO. (I can help here if you want)

OpenRA.Mods.Common/UtilityCommands/ExtractEmmyLuaAPI.cs Outdated Show resolved Hide resolved
penev92
penev92 previously approved these changes May 2, 2022
Copy link
Member

@penev92 penev92 left a comment

Choose a reason for hiding this comment

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

I have spent a great deal of time trying to understand how this, our Lua API, EmmyLua and the generated files work, I've helped out with the changes, I've done a bunch of testing and best of all - the Lua file generated by this is already "live in production" thanks to the OpenRA Lua extension, so it's pretty obvious exactly how well it all works together.
With that I give it a 👍 and because I know reviews are hard to get these days IMO we just need a code style review for +2.

@penev92
Copy link
Member

penev92 commented May 12, 2022

My 👍 still stands (despite the SECTION comments being gone).

@penev92
Copy link
Member

penev92 commented May 22, 2022

Some deprecated methods were removed and so this needs a rebase.

@Mailaender
Copy link
Member Author

All deprecated methods were removed...

Copy link
Member

@atlimit8 atlimit8 left a comment

Choose a reason for hiding this comment

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

The Game to utility changes are minor nits.

OpenRA.Mods.Common/UtilityCommands/ExtractEmmyLuaAPI.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/UtilityCommands/ExtractEmmyLuaAPI.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/UtilityCommands/ExtractEmmyLuaAPI.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/UtilityCommands/ExtractEmmyLuaAPI.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/UtilityCommands/ExtractEmmyLuaAPI.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/UtilityCommands/ExtractEmmyLuaAPI.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/UtilityCommands/ExtractEmmyLuaAPI.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/UtilityCommands/ExtractEmmyLuaAPI.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/UtilityCommands/ExtractEmmyLuaAPI.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/UtilityCommands/ExtractEmmyLuaAPI.cs Outdated Show resolved Hide resolved
@Mailaender
Copy link
Member Author

Updated

@atlimit8 atlimit8 merged commit 3f328a1 into OpenRA:bleed May 25, 2022
@atlimit8
Copy link
Member

Changelog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants