Adding program environment

[suhedge]

As solution for #1797 i was on my way to add program environment.
Currently (i did so) only shell uses that, mainly because there already too many changes for one submition.
Also some preparations for changing fs, like fs.combine() and fs.getDir() save prefixed '/'.

All changes are in bios.lua and rom.
In readme.txt all big enough changes.

almostprogenv2 is not breaking anything (i thing so) but also not adding.

Things are datapacks because that was easiest and fastest iteration loop.
datapacks.zip
Saved multiple versions as changes become too wide.

As i explored things and frequently seen them, i changed code style
sorry about that, i had fun.

next is readme.txt

started from 1.111.0

order of apperance

1. refactor
2. refactor2
3. beforemod
4. almostprogenv
5. almostprogenv2
6. progenv
7. lua -- merge with 1.114.2

maybe breaking changes:

-- dofile(file) -> dofile(file, ...)
-- fs.combine and fs.getDir save prefixed '/'
-- exception.try:
-- from: ok, err, co = try(func, ...)
-- to: ok, err = try(co, ...)

done big:

-- os.loadAPI() : expects return instead of _ENV set by api, (old behaviour saved as fallback)

-- new:
-- os.newproc(func)
-- os.getprocenv()
-- both shell and multishell using them, kind of

-- vector: API table could have functions of vectors (see XXXX)

-- io: handle now garbage collected

-- exception.try:
-- from: ok, err, co = try(func, ...)
-- to: ok, err = try(co, ...)

-- dofile(file) -> dofile(file, ...) -- in script accesible as '...'

-- fs.combine and fs.getDir save prefixed '/'

-- prepared fs for change

done small:

-- ALOT of style changes, mainly in apis

-- peripheral:
-- argnames peripheral -> periph
-- peripheral.find was outlier (used (_ENV or _G).peripheral) (no more)

-- settings.define: code block swap (should not change behaviour)
-- settings.getNames: ;tab[n], n = val, n + 1; -> ;table.insert(tab, val); (ghosts remained) (see XXXX)
-- settings.load: shortcut possible (see XXXX)

-- window: local of globals (see XXXX)
-- window: window.reposition have weird if (see XXXX)

-- programs/lua: ghost remained (see XXXX)

-- when booting its freaking luck that when everything runs they have all globals they need
-- predefined order fits better

for revision:

-- print() and textutils.pagedPrint() dont need return value (at least not used in rom)
-- Descriptions probably outdated in some places

-- last minute i found out that completion for "pastebin" in progenv broken
-- guesing that it not only thing that broken

[Wojbie]

Is this a bug report or pull request? Because if it's a data pack showcase it would fit more in discussion page then in issues.

If it's pull request then it definitely seems to be breaking backwards compatibility.

[fatboychummy]

This all feels... Very out of scope for fixing #1797...

-- dofile(file) -> dofile(file, ...)

Why are we changing a default Lua function?

-- exception.try: -- from: ok, err, co = try(func, ...) -- to: ok, err = try(co, ...)

What was wrong with the current implementation?

-- os.loadAPI() : expects return instead of _ENV set by api, (old behaviour saved as fallback)

loadAPI is deprecated, and people should be using require instead. Updating it to modern standards is likely a bad idea if we want to push people to use require.

-- new: -- os.newproc(func) -- os.getprocenv() -- both shell and multishell using them, kind of

Assuming this is creating background processes, again very out of scope.

-- vector: API table could have functions of vectors (see XXXX)

This will break things, because it allowed you to do things like this:

vector.add(v1, v2)

Or similar, without needing actual vector objects.

-- io: handle now garbage collected

Honestly don't see what you changed here (but am on mobile so hard to diff these)

-- settings.getNames: ;tab[n], n = val, n + 1; -> ;table.insert(tab, val); (ghosts remained) (see XXXX)

By definition of how tables work, k cannot be nil. This means there would be no nil values in the resultant table.

-- window: local of globals (see XXXX)

Performance benefits. It's faster to run locals than to run globals, and window methods use these funcs a lot.

-- window: window.reposition have weird if (see XXXX)

This if statement tests if either newWidth or newHeight exist. If one does, then it ensures the other does as well. It allows you to do reposition(x, y) as well as reposition(x, y, width, height), but not reposition(x, y, width). If width is specified, so must height (and vice versa).

-- programs/lua: ghost remained (see XXXX)

Since this is the only method used, and it's only used once, it's fine to require and run in-line like this.

-- when booting its freaking luck that when everything runs they have all globals they need -- predefined order fits better

I'm fairly sure the boot process is rather predefined in order already, is it not?

-- print() and textutils.pagedPrint() dont need return value (at least not used in rom) -- Descriptions probably outdated in some places

I personally use this return value! It is occasionally useful to know how many lines have been written to the screen.

---

Overall, this change would break a lot of programs, I agree with Wojbie.

---

Edit: Ah, I see what os.newproc is doing now. It's converting a function into a coroutine, and storing the env for it. This is likely why exception.try also changed.

[fatboychummy]

A seperate branch was created by @MCJack123 to help diff the files: mc-1.20.x...MCJack123:CC-Tweaked:issue-2045

[suhedge]

-- dofile(file) -> dofile(file, ...)

Why are we changing a default Lua function?

Good point

-- os.loadAPI() : expects return instead of _ENV set by api, (old behaviour saved as fallback)

loadAPI is deprecated, and people should be using require instead. Updating it to modern standards is likely a bad idea if we want to push people to use require.

Its hard to find where globals set
-- textutils.someconst = ...
easier to find than
-- someconst = ...
Also it seems that this scheme is used in most places, so the goal is similarity

-- io: handle now garbage collected

Honestly don't see what you changed here (but am on mobile so hard to diff these)

Oh yea somehow that change was lost.
it was:
defined handle metatable.__gc() which acts as metatable.__index.close

-- when booting its freaking luck that when everything runs they have all globals they need -- predefined order fits better

I'm fairly sure the boot process is rather predefined in order already, is it not?

Its predefined by fs.list(): (from bios.lua)

local function load_apis(dir)
    if  not fs.isDir(dir) then return end

    for _, file in ipairs(fs.list(dir)) do -- <<< here
        if  (file:sub(1, 1) ~= ".") then
            local path = fs.combine(dir, file)
            if  not (fs.isDir(path) or os.loadAPI(path)) then
                bAPIError = true
            end
        end
    end
end

example of dependency is apis/colours.lua:

local colours = {}
for k, v in pairs(colors) do -- <<< here colors used
    colours[k] = v
end

why it works? because fs.list() places "colors.lua" before "colours.lua"

-- vector: API table could have functions of vectors (see XXXX)

This will break things, because it allowed you to do things like this:

vector.add(v1, v2)

Or similar, without needing actual vector objects.

Currently not a thing, i suspect you confused what was and what changed.
I suggest exactly that.

If it's pull request then it definitely seems to be breaking backwards compatibility.

The only changes that definetly breaks backwards compatibility is fs.combine() and fs.getDir().
Yea it probably should be pull request

now i probably will condense all nonbreaking changes

[fatboychummy]

Oh yea somehow that change was lost. it was: defined handle metatable.__gc() which acts as metatable.__index.close

I am fairly sure that CC doesn't support __gc, but cannot remember off the top of my head. At the very least, I cannot get it to trigger in any of my testing.

CC does properly garbage-collect these handles when the computer shuts down though. That is handled internally.

Its predefined by fs.list(): (from bios.lua)

local function load_apis(dir)
    if  not fs.isDir(dir) then return end

    for _, file in ipairs(fs.list(dir)) do -- <<< here
        if  (file:sub(1, 1) ~= ".") then
            local path = fs.combine(dir, file)
            if  not (fs.isDir(path) or os.loadAPI(path)) then
                bAPIError = true
            end
        end
    end
end

example of dependency is apis/colours.lua:

local colours = {}
for k, v in pairs(colors) do -- <<< here colors used
    colours[k] = v
end

why it works? because fs.list() places "colors.lua" before "colours.lua"

Yes, fs.list() has a specific return order that is guaranteed.

Currently not a thing, i suspect you confused what was and what changed. I suggest exactly that.

Sorry, I seem to have forgotten how the vector library was structured. My fault.

[fatboychummy]

I still want to emphasize that this all feels quite out of scope though. After now taking some more time to review it, I've noticed that the changes don't seem fully well-aligned to the issue's purpose. The issue title is about adding program environments, but the proposed code does so much more, including (but probably not limited to):

• Refactoring every single API file to be require style.
• Refactoring os.loadAPI to support require style libraries, despite os.loadAPI being deprecated.
• Adding numerous debug print statements on startup, which are unnecessary.
• Adding the __gc metamethod to io handles.
• Implementing the proposed environments (like suggested).
• Altering fs to allow for absolute paths (like suggested).
• Changing the system boot process.
• Refactoring every single Lua file (again) to match your personal style.

Don't get me wrong, it's nice to see people with this kind of zeal for a project like CC:T, it's just that many of those things should be handled through other PRs, rather than being clumped into one gigantic PR. Not only are your chances of a merge much higher if you handle issues one-at-a-time, but it'll be much easier and quicker for Squid to come to a decision about such a PR.

The style changes, in particular, should be well-discussed, rather than just being tossed haphazardly into another PR about something else. Those affect everything, from current maintainers to future maintainers, and can also cause troubles with currently open PRs.

[MCJack123]

I was going to stay out of this discussion, but since I'm now getting emails about it, I'll add my two cents.

My mindset when criticizing peoples' code isn't to try to put them down, or make them feel bad. My goal is to tell people the blunt truth while keeping optimism - I hate pushing people away from things they worked hard on, but at the same time, they need to be aware of their own shortcomings, which avoids even harsher criticism from not-so-nice actors.

I think file paths are fine as-is: keeping everything at the root keeps the logic consistent. You don't need to worry about whether a path you're receiving is absolute or relative, and neither does the system. If you need a relative path, likely from user input, you ask the shell. CraftOS is a very simple operating system (if you can even call it one). Don't ask it to do more than it needs; if you need more, you find another library or OS, or you write it out yourself.

Blasting out +2084/-1422 changes is a lot for a single review. Enhancement requests should be very precise as to what they do. Coders aren't lawmakers, they hate when a bunch of unrelated nonsense is tacked onto a good feature. If you want to make multiple changes, you make separate requests for each change, each ideally independent of another (if not possible, then mark the dependent as a draft/blocked until the dependency is merged).

And please learn how to use Git(Hub) - this is not to be mean, but one of the primary features of GitHub is that you can fork code, edit it, and then make a pull request to merge it, and using a ZIP of ZIPs of versions posted in a plain issue suggests to me that you aren't very familiar with the tools. It helps a lot to be able to see the changes compared to current, which is why I silently put the changes on my own fork so I and other contributors could peek through it.

As already mentioned, there's a bunch of cruft that'll very likely end up breaking things. CC:T takes pride in backwards compatibility (to the point that it's a meme), as messing with APIs will inevitably cause thousands of Minecrafters to cry that some program that used to work no longer works.

I'll also add that Squid is very stringent with style guidelines. It takes me two or three tries to pass linting on very minor PRs, so trying to overhaul anything style-wise will backfire spectacularly. You don't own the project, so you don't control how it's styled.

As it stands, it's highly unlikely that this code goes through, but that's okay! The only suggestions I've made that haven't needed back-and-forth tweaking were single line typo fixes. There'd likely have to be a lot more back-and-forth about the best way to go about this change without breaking anyone's code, and while keeping the API clean.

And if you prefer doing it your way, you can always spin off your own version of CraftOS for personal use. For example, I don't like the CraftOS shell very much, so I decided to make my own, which has a bunch of extra features for myself to use. I know it breaks a lot of shell things, so I keep it separate from CC:T's code, and I distribute it on my own. My opinion is that you should write code for you first, and others second, which avoids being disappointment if (when) nobody else uses it.

I hope I or the other commenters don't discourage you from making cool code, but when dealing with other people and their code, there's always gonna be some pushback to make sure only the best code makes it in. Of course, none of us have any real authority, and you might as well just dismiss everything I've posted here because in the end, Squid's the one calling the shots. But I am a serial yapper, and I want to raise my concerns as someone who's worked inside and out of CC for years.

[suhedge]

Thank you, man!
I appreciate your word.
I had a lot of thoughts, almost none got here.

I opened this issue because i was doing something that could be usefull,
so no hard feeling (currently only warm feeling).

As already mentioned, there's a bunch of cruft that'll very likely end up breaking things. CC:T takes pride in backwards compatibility (to the point that it's a meme), as messing with APIs will inevitably cause thousands of Minecrafters to cry that some program that used to work no longer works.

I finished version with full backwards compatibility but considering all, i just leave here some findings.

findings

findings.zip

io: handleMetatable.read() has redundant if (see XXXX)

rednet: rednet.run() has maybe redundant expressions (see XXXX)

And a last, suggestions

I thing closing closed file shouldnt raise error, return error message at most

Would be nice to have vector functions inside _G.vector

sometimes files dont closes (not io related) (i will open separate issue)

[fatboychummy]

rednet: rednet.run() has maybe redundant expressions (see XXXX)

local nan, three = 0/0, 3

print(type(nan), type(three)) --> number number

print(nan == nan, three == three) --> false true

nan retains its number type (despite being defined as literally "not a number"). However, a distinct quality of nan is that it does not equal itself. Thus, this is used to detect if the message ID or sender ID is nan.

[SquidDev]

Thank you for looking at this!

I've applied a couple of the smaller bug-fixes (i.e. ad74893), however I'm afraid I don't think much more of this is especially integrateable. The hard bit about #1797 is doing this in a backwards compatible way (I suspect it's just impossible, but not something I want to discount entirely, hence the issue staying open for now) and, as others have mentioned, this is something I take pretty seriously with CC:T.