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

To meet module naming convention #194

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nstoker
Copy link

@nstoker nstoker commented May 14, 2017

This patch is aimed at fixing an error with embedded module names containing hyphens. Visual Studio converts the hyphen into an underscore, however the Moonsharp library does not replace the hyphen with an underscore, this prevents an embedded LUA script from being loaded.

According to Microsoft documentation (see
http://stackoverflow.com/questions/14705211/how-is-net-renaming-my-embedded-resources)
module names may not include hyphens and should be replaced by an
underscores.

According to Microsoft documentation (see
http://stackoverflow.com/questions/14705211/how-is-net-renaming-my-embedded-resources)
module names may not include hyphens and should be replaced by an
underscore.
@LimpingNinja LimpingNinja self-requested a review January 9, 2020 18:19
@LimpingNinja
Copy link
Contributor

Given the documentation:

"VerifyResourceName method compares each character in the string to a set of invalid tokens based on the language specified by the provider parameter. Any invalid character in the string is replaced with an underscore character. The characters that will be replaced with an underscore are as follows:

' ' (space), U+00A0 (non-breaking space), '.' (period), ',' (comma), ';' (semicolon), '|', '~', '@', '#', '%', '^', '&', '*', '+', '-', '/', '', '<', '>', '?', '[', ']', '(', ')', '{', '}', '"' (quote), ''' (apostrophe), ':', and '!'."

Should we not look at extending this to cover those scenarios? RIght now it seems like a bit of a simplistic patch to handle the single use case.

@nstoker
Copy link
Author

nstoker commented Apr 9, 2020

I have since changed companies, and no longer am doing any development in LUA (although that may change again in the future).

It would make sense to extend the fix to cover the other scenarios, but I currently don't have resources to look at them.

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 this pull request may close these issues.

2 participants