Some redesigning of the settings API (#408)

- The store is now split into two sections:
   - A list of possible options, with some metadata about them.
   - A list of values which have been changed.
 - settings.define can be used to register a new option. We have
   migrated all existing options over to use it. This can be used to
   define a default value, description, and a type the setting must have
   (such as `string` or `boolean).

 - settings.{set,unset,clear,load,store} operate using this value list.
   This means that only values which have been changed are stored to
   disk.
   Furthermore, clearing/unsetting will reset to the /default/ value,
   rather than removing entirely.

 - The set program will now display descriptions.

 - settings.{load,save} now default to `.settings` if no path is given.

src/main/resources/assets/computercraft/lua/bios.lua

@@ -881,18 +881,66 @@ if bAPIError then
 end
 
 -- Set default settings
-settings.set("shell.allow_startup", true)
+settings.define("shell.allow_startup", {
-settings.set("shell.allow_disk_startup", commands == nil)
+    default = true,
-settings.set("shell.autocomplete", true)
+    description = "Run startup files when the computer turns on.",
-settings.set("edit.autocomplete", true)
+    type = "boolean",
-settings.set("edit.default_extension", "lua")
+})
-settings.set("paint.default_extension", "nfp")
+settings.define("shell.allow_disk_startup", {
-settings.set("lua.autocomplete", true)
+    default = commands == nil,
-settings.set("list.show_hidden", false)
+    description = "Run startup files from disk drives when the computer turns on.",
-settings.set("motd.enable", false)
+    type = "boolean",
-settings.set("motd.path", "/rom/motd.txt:/motd.txt")
+})
+
+settings.define("shell.autocomplete", {
+    default = true,
+    description = "Autocomplete program and arguments in the shell.",
+    type = "boolean",
+})
+settings.define("edit.autocomplete", {
+    default = true,
+    description = "Autocomplete API and function names in the editor.",
+        type = "boolean",
+})
+settings.define("lua.autocomplete", {
+    default = true,
+    description = "Autocomplete API and function names in the Lua REPL.",
+        type = "boolean",
+})
+
+settings.define("edit.default_extension", {
+    default = "lua",
+    description = [[The file extension the editor will use if none is given. Set to "" to disable.]],
+    type = "string",
+})
+settings.define("paint.default_extension", {
+    default = "nfp",
+    description = [[The file extension the paint program will use if none is given. Set to "" to disable.]],
+    type = "string",
+})
+
+settings.define("list.show_hidden", {
+    default = false,
+    description = [[Show hidden files (those starting with "." in the Lua REPL)]],
+    type = "boolean",
+})
+
+settings.define("motd.enable", {
+    default = false,
+    description = "Display a random message when the computer starts up.",
+    type = "boolean",
+})
+settings.define("motd.path", {
+    default = "/rom/motd.txt:/motd.txt",
+    description = [[The path to load random messages from. Should be a colon (":") separated string of file paths.]],
+    type = "string",
+})
 if term.isColour() then
-    settings.set("bios.use_multishell", true)
+    settings.define("bios.use_multishell", {
+        default = true,
+        description = [[Allow running multiple programs at once, through the use of the "fg" and "bg" programs.]],
+        type = "boolean",
+    })
 end
 if _CC_DEFAULT_SETTINGS then
     for sPair in string.gmatch(_CC_DEFAULT_SETTINGS, "[^,]+") do

src/main/resources/assets/computercraft/lua/rom/apis/settings.lua

@@ -6,9 +6,86 @@
 --
 -- @module settings
 
-local expect = dofile("rom/modules/main/cc/expect.lua").expect
+local expect = dofile("rom/modules/main/cc/expect.lua")
+local type, expect, field = type, expect.expect, expect.field
 
-local tSettings = {}
+local details, values = {}, {}
+
+local function reserialize(value)
+    if type(value) ~= "table" then return value end
+    return textutils.unserialize(textutils.serialize(value))
+end
+
+local function copy(value)
+    if type(value) ~= "table" then return value end
+    local result = {}
+    for k, v in pairs(value) do result[k] = copy(v) end
+    return result
+end
+
+local valid_types = { "number", "string", "boolean", "table" }
+for _, v in ipairs(valid_types) do valid_types[v] = true end
+
+--- Define a new setting, optional specifying various properties about it.
+--
+-- While settings do not have to be added before being used, doing so allows
+-- you to provide defaults and additional metadata.
+--
+-- @tparam string name The name of this option
+-- @tparam[opt] { description? = string, default? = value, type? = string } options
+-- Options for this setting. This table accepts the following fields:
+--
+--  - `description`: A description which may be printed when running the `set` program.
+--  - `default`: A default value, which is returned by @{settings.get} if the
+--    setting has not been changed.
+--  - `type`: Require values to be of this type. @{set|Setting} the value to another type
+--    will error.
+function define(name, options)
+    expect(1, name, "string")
+    expect(2, options, "table", nil)
+
+    if options then
+        options = {
+            description = field(options, "description", "string", "nil"),
+            default = reserialize(field(options, "default", "number", "string", "boolean", "table", "nil")),
+            type = field(options, "type", "string", "nil"),
+        }
+
+        if options.type and not valid_types[options.type] then
+            error(("Unknown type %q. Expected one of %s."):format(options.type, table.concat(valid_types, ", ")), 2)
+        end
+    else
+        options = {}
+    end
+
+    details[name] = options
+end
+
+--- Remove a @{define|definition} of a setting.
+--
+-- If a setting has been changed, this does not remove its value. Use @{settings.unset}
+-- for that.
+--
+-- @tparam string name The name of this option
+function undefine(name)
+    expect(1, name, "string")
+    details[name] = nil
+end
+
+local function set_value(name, value)
+    local new = reserialize(value)
+    local old = values[name]
+    if old == nil then
+        local opt = details[name]
+        old = opt and opt.default
+    end
+
+    values[name] = new
+    if old ~= new then
+        -- This should be safe, as os.queueEvent copies values anyway.
+        os.queueEvent("setting_changed", name, new, old)
+    end
+end
 
 --- Set the value of a setting.
 --
@@ -21,43 +98,43 @@ function set(name, value)
     expect(1, name, "string")
     expect(2, value, "number", "string", "boolean", "table")
 
-    if type(value) == "table" then
+    local opt = details[name]
-        -- Ensure value is serializeable
+    if opt and opt.type then expect(2, value, opt.type) end
-        value = textutils.unserialize(textutils.serialize(value))
-    end
-    tSettings[name] = value
-end
 
-local copy
+    set_value(name, value)
-function copy(value)
-    if type(value) == "table" then
-        local result = {}
-        for k, v in pairs(value) do
-            result[k] = copy(v)
-        end
-        return result
-    else
-        return value
-    end
 end
 
 --- Get the value of a setting.
 --
 -- @tparam string name The name of the setting to get.
 -- @param[opt] default The value to use should there be pre-existing value for
--- this setting. Defaults to `nil`.
+-- this setting. If not given, it will use the setting's default value if given,
--- @return The setting's, or `default` if the setting has not been set.
+-- or `nil` otherwise.
+-- @return The setting's, or the default if the setting has not been changed.
 function get(name, default)
     expect(1, name, "string")
-    local result = tSettings[name]
+    local result = values[name]
     if result ~= nil then
         return copy(result)
-    else
+    elseif default ~= nil then
         return default
+    else
+        local opt = details[name]
+        return opt and copy(opt.default)
     end
 end
 
---- Remove the value of a setting, clearing it back to `nil`.
+--- Get details about a specific setting
+function getDetails(name)
+    expect(1, name, "string")
+    local deets = copy(details[name]) or {}
+    deets.value = values[name]
+    deets.changed = deets.value ~= nil
+    if deets.value == nil then deets.value = deets.default end
+    return deets
+end
+
+--- Remove the value of a setting, setting it to the default.
 --
 -- @{settings.get} will return the default value until the setting's value is
 -- @{settings.set|set}, or the computer is rebooted.
@@ -67,15 +144,17 @@ end
 -- @see settings.clear
 function unset(name)
     expect(1, name, "string")
-    tSettings[name] = nil
+    set_value(name, nil)
 end
 
---- Removes the value of all settings. Equivalent to calling @{settings.unset}
+--- Resets the value of all settings. Equivalent to calling @{settings.unset}
 --- on every setting.
 --
 -- @see settings.unset
 function clear()
-    tSettings = {}
+    for name in pairs(values) do
+        set_value(name, nil)
+    end
 end
 
 --- Get the names of all currently defined settings.
@@ -83,9 +162,12 @@ end
 -- @treturn { string } An alphabetically sorted list of all currently-defined
 -- settings.
 function getNames()
-    local result = {}
+    local result, n = {}, 1
-    for k in pairs(tSettings) do
+    for k in pairs(details) do
-        result[#result + 1] = k
+        result[n], n = k, n + 1
+    end
+    for k in pairs(values) do
+        if not details[k] then result[n], n = k, n + 1 end
     end
     table.sort(result)
     return result
@@ -96,15 +178,15 @@ end
 -- Existing settings will be merged with any pre-existing ones. Conflicting
 -- entries will be overwritten, but any others will be preserved.
 --
--- @tparam string sPath The file to load from.
+-- @tparam[opt] string sPath The file to load from, defaulting to `.settings`.
 -- @treturn boolean Whether settings were successfully read from this
 -- file. Reasons for failure may include the file not existing or being
 -- corrupted.
 --
 -- @see settings.save
 function load(sPath)
-    expect(1, sPath, "string")
+    expect(1, sPath, "string", "nil")
-    local file = fs.open(sPath, "r")
+    local file = fs.open(sPath or ".settings", "r")
     if not file then
         return false
     end
@@ -118,9 +200,12 @@ function load(sPath)
     end
 
     for k, v in pairs(tFile) do
-        if type(k) == "string" and
+        local ty_v = type(k)
-           (type(v) == "string" or type(v) == "number" or type(v) == "boolean" or type(v) == "table") then
+        if type(k) == "string" and (ty_v == "string" or ty_v == "number" or ty_v == "boolean" or ty_v == "table") then
-            set(k, v)
+            local opt = details[name]
+            if not opt or not opt.type or ty_v == opt.type then
+                set_value(k, v)
+            end
         end
     end
 
@@ -132,18 +217,18 @@ end
 -- This will entirely overwrite the pre-existing file. Settings defined in the
 -- file, but not currently loaded will be removed.
 --
--- @tparam string sPath The path to save settings to.
+-- @tparam[opt] string sPath The path to save settings to, defaulting to `.settings`.
 -- @treturn boolean If the settings were successfully saved.
 --
 -- @see settings.load
 function save(sPath)
-    expect(1, sPath, "string")
+    expect(1, sPath, "string", "nil")
-    local file = fs.open(sPath, "w")
+    local file = fs.open(".settings", "w")
     if not file then
         return false
     end
 
-    file.write(textutils.serialize(tSettings))
+    file.write(textutils.serialize(values))
     file.close()
 
     return true

src/main/resources/assets/computercraft/lua/rom/programs/set.lua

@@ -1,3 +1,4 @@
+local pp = require "cc.pretty"
 
 local tArgs = { ... }
 if #tArgs == 0 then
@@ -12,7 +13,13 @@ if #tArgs == 0 then
 elseif #tArgs == 1 then
     -- "set foo"
     local sName = tArgs[1]
-    print(textutils.serialize(sName) .. " is " .. textutils.serialize(settings.get(sName)))
+    local deets = settings.getDetails(sName)
+    local msg = pp.text(sName, colors.cyan) .. " is " .. pp.pretty(deets.value)
+    if deets.default ~= nil and deets.value ~= deets.default then
+        msg = msg .. " (default is " .. pp.pretty(deets.default) .. ")"
+    end
+    pp.print(msg)
+    if deets.description then print(deets.description) end
 
 else
     -- "set foo bar"
@@ -31,15 +38,18 @@ else
         value = sValue
     end
 
-    local oldValue = settings.get(sValue)
+    local option = settings.getDetails(sName)
-    if value ~= nil then
+    if value == nil then
-        settings.set(sName, value)
-        print(textutils.serialize(sName) .. " set to " .. textutils.serialize(value))
-    else
         settings.unset(sName)
         print(textutils.serialize(sName) .. " unset")
+    elseif option.type and option.type ~= type(value) then
+        printError(("%s is not a valid %s."):format(textutils.serialize(sValue), option.type))
+    else
+        settings.set(sName, value)
+        print(textutils.serialize(sName) .. " set to " .. textutils.serialize(value))
     end
-    if value ~= oldValue then
+
-        settings.save(".settings")
+    if value ~= option.value then
+        settings.save()
     end
 end

src/test/resources/test-rom/spec/apis/settings_spec.lua

@@ -1,4 +1,18 @@
 describe("The settings library", function()
+    describe("settings.define", function()
+        it("ensures valid type names", function()
+            expect.error(settings.define, "test.defined", { type = "function" })
+                :eq('Unknown type "function". Expected one of number, string, boolean, table.')
+        end)
+    end)
+    describe("settings.undefine", function()
+        it("clears defined settings", function()
+            settings.define("test.unset", { default = 123 })
+            settings.undefine("test.unset")
+            expect(settings.get("test.unset")):eq(nil)
+        end)
+    end)
+
     describe("settings.set", function()
         it("validates arguments", function()
             settings.set("test", 1)
@@ -13,6 +27,30 @@ describe("The settings library", function()
         it("prevents storing unserialisable types", function()
             expect.error(settings.set, "", { print }):eq("Cannot serialize type function")
         end)
+
+        it("setting changes the value", function()
+            local random = math.random(1, 0x7FFFFFFF)
+            settings.set("test", random)
+            expect(settings.get("test")):eq(random)
+        end)
+
+        it("checks the type of the value", function()
+            settings.define("test.defined", { default = 123, description = "A description", type = "number" })
+            expect.error(settings.set, "test.defined", "hello")
+                :eq("bad argument #2 (expected number, got string)")
+            settings.set("test.defined", 123)
+        end)
+
+        it("setting fires an event", function()
+            settings.clear()
+
+            local s = stub(os, "queueEvent")
+            settings.set("test", 1)
+            settings.set("test", 2)
+
+            expect(s):called_with("setting_changed", "test", 1, nil)
+            expect(s):called_with("setting_changed", "test", 2, 1)
+        end)
     end)
 
     describe("settings.get", function()
@@ -20,6 +58,43 @@ describe("The settings library", function()
             settings.get("test")
             expect.error(settings.get, nil):eq("bad argument #1 (expected string, got nil)")
         end)
+
+        it("returns the default", function()
+            expect(settings.get("test.undefined")):eq(nil)
+            expect(settings.get("test.undefined", "?")):eq("?")
+
+            settings.define("test.unset", { default = "default" })
+            expect(settings.get("test.unset")):eq("default")
+            expect(settings.get("test.unset", "?")):eq("?")
+        end)
+    end)
+
+    describe("settings.getDetails", function()
+        it("validates arguments", function()
+            expect.error(settings.getDetails, nil):eq("bad argument #1 (expected string, got nil)")
+        end)
+
+        it("works on undefined and unset values", function()
+            expect(settings.getDetails("test.undefined")):same { value = nil, changed = false }
+        end)
+
+        it("works on undefined but set values", function()
+            settings.set("test", 456)
+            expect(settings.getDetails("test")):same { value = 456, changed = true }
+        end)
+
+        it("works on defined but unset values", function()
+            settings.define("test.unset", { default = 123, description = "A description" })
+            expect(settings.getDetails("test.unset")):same
+                { default = 123, value = 123, changed = false, description = "A description" }
+        end)
+
+        it("works on defined and set values", function()
+            settings.define("test.defined", { default = 123, description = "A description", type = "number" })
+            settings.set("test.defined", 456)
+            expect(settings.getDetails("test.defined")):same
+                { default = 123, value = 456, changed = true, description = "A description", type = "number" }
+        end)
     end)
 
     describe("settings.unset", function()
@@ -27,17 +102,73 @@ describe("The settings library", function()
             settings.unset("test")
             expect.error(settings.unset, nil):eq("bad argument #1 (expected string, got nil)")
         end)
+
+        it("unsetting resets the value", function()
+            settings.set("test", true)
+            settings.unset("test")
+            expect(settings.get("test")):eq(nil)
+        end)
+
+        it("unsetting does not touch defaults", function()
+            settings.define("test.defined", { default = 123 })
+            settings.set("test.defined", 456)
+            settings.unset("test.defined")
+            expect(settings.get("test.defined")):eq(123)
+        end)
+
+        it("unsetting fires an event", function()
+            settings.set("test", 1)
+
+            local s = stub(os, "queueEvent")
+            settings.unset("test")
+            expect(s):called_with("setting_changed", "test", nil, 1)
+        end)
+    end)
+
+    describe("settings.clear", function()
+        it("clearing resets all values", function()
+            settings.set("test", true)
+            settings.clear()
+            expect(settings.get("test")):eq(nil)
+        end)
+
+        it("clearing does not touch defaults", function()
+            settings.define("test.defined", { default = 123 })
+            settings.set("test.defined", 456)
+            settings.clear()
+            expect(settings.get("test.defined")):eq(123)
+        end)
+
+        it("clearing fires an event", function()
+            settings.set("test", 1)
+
+            local s = stub(os, "queueEvent")
+            settings.clear()
+            expect(s):called_with("setting_changed", "test", nil, 1)
+        end)
     end)
 
     describe("settings.load", function()
         it("validates arguments", function()
-            expect.error(settings.load, nil):eq("bad argument #1 (expected string, got nil)")
+            expect.error(settings.load, 1):eq("bad argument #1 (expected string, got number)")
+        end)
+
+        it("defaults to .settings", function()
+            local s = stub(fs, "open")
+            settings.load()
+            expect(s):called_with(".settings", "r")
         end)
     end)
 
     describe("settings.save", function()
         it("validates arguments", function()
-            expect.error(settings.save, nil):eq("bad argument #1 (expected string, got nil)")
+            expect.error(settings.save, 1):eq("bad argument #1 (expected string, got number)")
+        end)
+
+        it("defaults to .settings", function()
+            local s = stub(fs, "open")
+            settings.save()
+            expect(s):called_with(".settings", "w")
         end)
     end)
 end)

src/test/resources/test-rom/spec/programs/set_spec.lua

@@ -1,29 +1,59 @@
 local capture = require "test_helpers".capture_program
 
 describe("The set program", function()
+    local function setup()
+        local set = setmetatable({}, { __index = _G })
+        loadfile("/rom/apis/settings.lua", set)()
+        stub(_G, "settings", set)
+
+        settings.set("test", "Hello World!")
+        settings.define("test.defined", { default = 456, description = "A description", type = "number" })
+    end
 
     it("displays all settings", function()
-        settings.clear()
+        setup()
-        settings.set("Test", "Hello World!")
-        settings.set("123", 456)
 
         expect(capture(stub, "set"))
-            :matches { ok = true, output = '"123" is 456\n"Test" is "Hello World!"\n', error = "" }
+            :matches { ok = true, output = '"test" is "Hello World!"\n"test.defined" is 456\n', error = "" }
     end)
 
-    it("displays a single settings", function()
+    it("displays a single setting", function()
-        settings.clear()
+        setup()
-        settings.set("Test", "Hello World!")
-        settings.set("123", 456)
 
-        expect(capture(stub, "set Test"))
+        expect(capture(stub, "set test"))
-            :matches { ok = true, output = '"Test" is "Hello World!"\n', error = "" }
+            :matches { ok = true, output = 'test is "Hello World!"\n', error = "" }
+    end)
+
+    it("displays a single setting with description", function()
+        setup()
+
+        expect(capture(stub, "set test"))
+            :matches { ok = true, output = 'test is "Hello World!"\n', error = "" }
+    end)
+
+    it("displays a changed setting with description", function()
+        setup()
+
+        settings.set("test.defined", 123)
+        expect(capture(stub, "set test.defined"))
+            :matches { ok = true, output = 'test.defined is 123 (default is 456)\nA description\n', error = "" }
     end)
 
     it("set a setting", function()
-        expect(capture(stub, "set Test Hello"))
+        setup()
-            :matches { ok = true, output = '"Test" set to "Hello"\n', error = "" }
 
-        expect(settings.get("Test")):eq("Hello")
+        expect(capture(stub, "set test Hello"))
+            :matches { ok = true, output = '"test" set to "Hello"\n', error = "" }
+
+        expect(settings.get("test")):eq("Hello")
+    end)
+
+    it("checks the type of a setting", function()
+        setup()
+
+        expect(capture(stub, "set test.defined Hello"))
+            :matches { ok = true, output = "", error = '"Hello" is not a valid number.\n' }
+        expect(capture(stub, "set test.defined 456"))
+            :matches { ok = true, output = '"test.defined" set to 456\n', error = "" }
     end)
 end)