Quenty / NevermoreEngine

ModuleScript loader with reusable and easy unified server-client modules for faster game development on Roblox
https://quenty.github.io/NevermoreEngine/
MIT License
385 stars 123 forks source link

Should modules like os module import for a single function or duplicate code? #33

Closed Validark closed 8 years ago

Validark commented 8 years ago

The os module needs to use the overflow function of the table module twice. Should it be retrieved from the table module or should it just be copy and pasted over?

Quenty commented 8 years ago

I'd like to not override the os module and instead add a different namespace to prevent confusion / linters from failing when using Nevermore.

Just localize the variable in the script if you think it's worth the extra code.

Validark commented 8 years ago

While adding a new namespace would be sufficient, in the case of Roblox Devs, it wouldn't be helpful. My os module mimics the default Lua one. It still keeps os.time() and os.difftime(), it only adds back what should already be there.

By using syntax that is familiar to Lua users, this only makes the module make more sense.

Validark commented 8 years ago

It's similar to how you add a :Destroy() syntax to custom objects

Quenty commented 8 years ago

I'm glad you're debating this, because it should be up for debate.

I want to keep code in Nevermore as reusable as possible. This means that users should easily be able to extract what modules are required. Although we want to follow ROBLOX syntax, or even extend it (as I do with :Destroy() on custom objects), we do want to avoid overwriting native libraries, instead we want to extend these libraries and then make it clear.

If the user wants, they can do

local table = setmetatable(NevemoreEngine.LoadLibrary("qTable"), {__index = table})

To extend the existing table library. So too do I expect users to be able to extend your existing os library if they so need.

Which, by the way, you can use a metatable instead of doing this:

function lib.time()
     return os.time()
end

You can do this

setmetatable(lib, {__index = is})

In any event, I think it's important that we address this issue now and come to a conclusion.

I've reopened the issue so we can resolve this issue.

Validark commented 8 years ago

I edited the os module to use setmetatable. I think it's ready for integration

Quenty commented 8 years ago

It's been integrated.