Afforess / Factorio-Stdlib

Factorio Standard Library Project
ISC License
165 stars 49 forks source link

Local cleanup #73

Closed Nexela closed 7 years ago

Nexela commented 7 years ago

Not Completely ready But it is available for comments!

Nexela commented 7 years ago

Busted checks for the inventory thing need to be fixed but I am not versed enough to do it. stack in the spec will return 0 since the count is decremented directly from the stack table?

Afforess commented 7 years ago

Since this is a breaking change already - It would be nice if you could remove all of the code that has already been noted as deprecated.

Afforess commented 7 years ago

I understand the intent of moving the modules to not store functions / data in the global Lua environment, agree it's a solid change that improves the stdlib, but I also worry it might make the stdlib harder for beginners to understand how to use. I think we can compromise and also provide a helper entrypoint (stdlib.lua?) which requires all of the files and exposes them to the global Lua environment, essentially reproducing the same behavior as before. Then modders can choose require the modules individually or require all of them at once, e.g:

stdlib.lua

Area = require 'stdlib/area/area'
Position = require 'stdlib/area/position'
Tile = require 'stdlib/area/tile'
...

(Is it possible to iterate over the files in a 'package' in Lua? Should we consider generating this file programmatically via make or some other tool, so new files are not left out?)

If this is a bad idea for some reason though, or you have a better suggestion, feel free to let me know. I'm just trying to think through how I'm going to recommend people update for the breaking changes, and anything to simplify this process and make it less burdonsome is important.

Nexela commented 7 years ago

RE local vr global Keeping the module deceleration global might be the better option. This would allow for backwards compatibility with Current setups without affecting performance. Allowing for both require("module") and Module = require("module") This would also keep the number of "breaking" changes to a minimum.

As for the stdlib.lua something like that would help with beginners but at the same time they would be learning bad coding habits. Also have to take into consideration what modules to put on the autoload list. I think the better option would be a readme expansion and maybe a short how to use comment section at the top of each file.

RE deprecated, Is that in reference to all of the deprecated stuff as well as inventory module. My vote is inventory stays but with a new name for the function that better describes it.

Nexela commented 7 years ago

As for the event system, I am thinking moving the _registry into _G and doing a newindex meta table on Event to point to it. might be a feasible option and would still allow mods/modules to do Event.whatever = "" not sure how I feel about that.

Afforess commented 7 years ago

If you want to keep the module declarations global, that also solves the issue. If we do that then I don't think a stdlib.lua entrypoint is needed.

RE deprecated, Is that in reference to all of the deprecated stuff as well as inventory module. My vote is inventory stays but with a new name for the function that better describes it.

Implement it and that sounds fine.

As for the event system, I am thinking moving the _registry into _G and doing a newindex meta table on Event to point to it. might be a feasible option and would still allow mods/modules to do Event.whatever = ""

I'm not sure I understand... can you explain what the benefits of this would be with an example?

Afforess commented 7 years ago

@Nexela I'm good with this PR as-is? Are you ready for it to be merged?

Nexela commented 7 years ago

This PR can be merged. Any future work I will do another PR for. Thanks!

luacheckrc updated. My next phase will be adding all of the STDlib stuff into it. Total: 0 warnings / 0 errors in 22 files