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

fix: return module #3

Merged
merged 5 commits into from
Oct 26, 2021
Merged

fix: return module #3

merged 5 commits into from
Oct 26, 2021

Conversation

pintsized
Copy link
Contributor

@pintsized pintsized commented Jun 22, 2021

Previously loading this module would set pack and unpack into the global string library. Since these methods now exist (since Lua 5.3), other Lua modules will use these functions if they exist, assuming that they are the implementations in Lua 5.3.

The upshot being that in various places where this module is used, the user is forced to unload it and reset the string methods in order to avoid breaking other code.

More generally, modern best practice is to return the module when loading instead of setting anything globally.

Obviously this is a breaking change, and would require a major version bump.

Previously loading this module would set `pack` and `unpack` into the
global `string` library. Since these methods now exist (since Lua 5.3),
other Lua modules will use these functions if they exist, assuming that
they are the implemenations in Lua 5.3.

More generally, modern best practice is to return the module when
loading instead of setting anything globally.

Obviously this is a breaking change.
@CLAassistant
Copy link

CLAassistant commented Jun 22, 2021

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@Tieske Tieske left a comment

Choose a reason for hiding this comment

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

lgtm

Use `luarocks build` or `luarocks make` instead.  The "builtin" process
is much better at providing the appropriate CFLAGS.
Still not a real test, since it doesn't show failure, but at least now
it runs to completion.
Since it no longer pollutes the `string` table, it's just a normal
module.  Also use a bit of formatting.
@javierguerragiraldez javierguerragiraldez merged commit 495bf30 into master Oct 26, 2021
@javierguerragiraldez javierguerragiraldez deleted the feat/return-module branch October 26, 2021 22:17
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.

5 participants