monome / norns

norns is many sound instruments.
http://monome.org
GNU General Public License v3.0
630 stars 147 forks source link

protect system tables from (accidental) overwrite #912

Closed pq closed 4 years ago

pq commented 5 years ago

follow-up from (#559), write-protect specific system tables:

...

/cc @tehn

tehn commented 4 years ago

@pq (i know you're away, so no rush!) wondering how to best approach this with other tables

ie, main user error that will happen is something like:

metro = 5

which plows the metro lib. but of course this gets reset on script reload, so maybe this isn't an issue beyond throwing a confusing error to the user.

however

norns = "rad"

will plow the script restart function.

probably not worth the effort to protect the depths of the table, but maybe worth protecting (with a nice error message) just the top level table for various things that may be accidentally overwritten.

what do you think?

pq commented 4 years ago

i'm all for write-protecting common top-level tables (at least). sub-tables should be just as easy and probably worth guarding a few too.

have we seen any issues w/ this approach? (sorry: can't recall!)

tehn commented 4 years ago

main issue with protecting some subtables is if they have elements that are supposed to be writable by design (i realize this is probably bad style)--- i'd need to go through and identify these.

perhaps we just make another function that read-only's the top-level table?

pq commented 4 years ago

main issue with protecting some subtables is if they have elements that are supposed to be writable by design (i realize this is probably bad style)--- i'd need to go through and identify these.

it looks like we anticipated this 🎉 and there's actually precedent w/ engine.name

https://github.com/monome/norns/blob/96311a8834b866e08f6386c909e2d18b3325ead4/lua/core/startup.lua#L21

exception logic is here:

https://github.com/monome/norns/blob/96311a8834b866e08f6386c909e2d18b3325ead4/lua/lib/tabutil.lua#L206-L210

perhaps we just make another function that read-only's the top-level table?

sure. or if the list of exceptions is short, maybe just explicitly list them like we did for engine?

happy to help with either approach. 👍