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 sanity checks to the Lua script trait #20898

Merged
merged 1 commit into from Jun 11, 2023

Conversation

Mailaender
Copy link
Member

@Mailaender Mailaender commented Jun 3, 2023

This catches some basic errors such as wrong trait location and case-sensitive file name pitfalls.

abcdefg30
abcdefg30 previously approved these changes Jun 3, 2023
anvilvapre
anvilvapre previously approved these changes Jun 3, 2023
Copy link
Contributor

@anvilvapre anvilvapre left a comment

Choose a reason for hiding this comment

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

CheckLuaScriptFileExistance?

Or would you add future other LuaScript validations also to this class.

No checks on strange or dangerous filenames? Like empty, spaces, dots and path characters, non utf8? Or not needed?

OpenRA.Mods.Common/Lint/CheckLuaScript.cs Outdated Show resolved Hide resolved
@PunkPun
Copy link
Member

PunkPun commented Jun 3, 2023

I think it would make sense to include this in the server lint pass as well

@Mailaender
Copy link
Member Author

I thought about maybe migrating the Lua syntax check in here because we can check against the API, but not now.

@PunkPun
Copy link
Member

PunkPun commented Jun 3, 2023

For me this lint check is case insensitive. I suppose CI will trigger errors but my PC doesn't

@Mailaender
Copy link
Member Author

macOS and Windows are case-insensitive.

@PunkPun
Copy link
Member

PunkPun commented Jun 3, 2023

by default yes, what I meant was shouldn't the lint always be case sensitive?

@Mailaender
Copy link
Member Author

I guess it uses the native file system to load files and that is why.

@Mailaender
Copy link
Member Author

Sorry, I won't reimplement operating system file systems for this. It is clearly nonsensical and out of scope.

@PunkPun
Copy link
Member

PunkPun commented Jun 11, 2023

Then it's not much of a sanity check. I thought the main purpose was to check case discrepancies

Copy link
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

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

Anyhow, CI tests should catch it and that's enough to warrant a merge

@PunkPun PunkPun merged commit c31f2ab into OpenRA:bleed Jun 11, 2023
3 checks passed
@PunkPun
Copy link
Member

PunkPun commented Jun 11, 2023

Changelog

@Mailaender Mailaender deleted the check-luascript branch June 11, 2023 08:22
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

4 participants