Config Module about factorio-stdlib HOT 33 CLOSED

"^%[-?%d*(%.%d+)?%]$" <- black magic to me. lol

Ah yes, the beauty that are regular expressions (or just pattern matching in the case of lua)... Though in my experience they are more alchemy than black magic. You just keep adding and removing ingredients until you find out how to make gold 😄 The above one simply looks for any numeric value inside square brackets from the beginning to the end of the string.

I think the reservation of those special characters would be a good thing to future proof the module without having to worry about backwards compatibility.

Concerning the syntax parsing, I think you overlooked a sentence of mine. Basically, because of intuitiveness, I want the syntax for the user to be some/array[1][1], but internally we would still handle some/array/[1]/[1|, which is of course easier to parse.

A simple string.gsub(path, "%[", "/[") would transform the former path in the latter one. (Where / is actually replaced by the path separator. After that splitting into key segments is no problem any more.

Overall I'd suggest to draw inspiration form JSONPath which is a lot nearer to how lua tables store data than my first suggestion, XML itself. So that in the end we could maybe have something in the likes of

-- return the array
Config.get("some/array")
-- return value in array at index 1/table at key 1
Config.get("some/array[1]")
-- return key from second table in array
Config.get("some/array[2]/key")
-- return first value of array with index 1 in parent array
Config.get("some/array[1][1]")
-- return array of values with specified indices
Config.get("some/array[1,2,...]")
-- return array of values with index 1 through 5
Config.get("some/array[1:5]")
-- return array of second to last value in array
Config.get("some/array[2:]")
-- return array of first 5 values of array
Config.get("some/array[:5]")
-- return array of last to values in array
Config.get("some/array[-2:]")
-- return length of array
Config.get("some/array/length()")
-- return min numeric value of array
Config.get("some/array/min()")
-- return max numeric value of array
Config.get("some/array/max()")
-- return average numeric value of array
Config.get("some/array/avg()")

from factorio-stdlib.

Would you include array access? There is no real difference between the following calls

Config.get("some/path/to/array")[1]
Config.get("some/path/to/array[1]")

but I feel the latter options looks a bit cleaner and consistent and also may allow for some further additions to query possibilities similar to XPath, like

-- Return array of values 1 through 3
Config.get("some/path/to/array[1-3]")
-- Return last value in array
Config.get("some/path/to/array[last()]")

Also, I'd find a something like the following helpful in some situations

-- Returns true or false
Config.isset("some/path/to/variable")

from factorio-stdlib.

This Config module of mine is actually a conversion of a conversion from the original Bukkit Configuration implementation from 2010 or somewhere around there. Some of which was lost in translation, intentionally removed (intended for my own use after all) or not needed anymore. As a result there isn't much left or even applicable in Factorio. Mostly anything to do with storage mediums and the translation from that to usable variables. I'll by all means add back in what is missing or desired depending on what I'm capable of. Others are free to contribute additions for whatever I fail at. :D

One thing I think I'd fail at is your XPath stuff. I'm pretty bad at string parsing. Not even going to show what /my/ string parser looked like. Going to use the one @Choumiko added just yesterday so much easier. lol

I can certainly add isset. Currently I do

local temp = Config.get("whatever/here")
if temp ~= nil then
    --do whatever with 
else
    -- oh no temp isn't set
end

Once you mentioned isset I realized that if you specify a default to get() you'll never know for certain if it is really set or the default value returned. isset would solve that immediately.

from factorio-stdlib.

I'd be happy to try to extend such a module with a bit more functionality if desired, it was more of an idea that needed some peer review and input. In my opinion at least basic array support would probably be beneficial to modding, anything more I'm not completely sure about.

An isset method also serves as a nice shorthand in conditional statements when using the Config module for simple flags set on objects.

from factorio-stdlib.

I've pushed a PR of the initial version with a will fail test. This module is modeled after the stdlib/log/logger module. I have to leave for work so i haven't tested it at all.

from factorio-stdlib.

Config.get("some/path/to/array[1]")

Due to the beauty of LUA that is a value table value.

--The Config module way.
Config.set("some/path/to/array[1]", true)

--The manual way
cfg = {some = {path = {to = {}}}
cfg.some.path.to["array[1]"] = true

I think the easier solution would be to add another getter of some sort that would expect such things.
Something like

--get_array needs better name
Config.get_array("some/path/to/array", {1})
Config.get_array("some/path/to/array", {1,4})
Config.get_array("some/path/to/array", Config.last)

from factorio-stdlib.

The extra array getter has one disadvantage. There is no way to do something like the following:

Config.get("some/array[1]/value")

This is not only usable for arrays but for normal tables, too. In your current implementation I could add a table with numeric keys as data and would not be able to access that data through a config path, as all segments of the path are treated as string keys. But in lua table["2"] and table[2] are distinct keys that can exist in the same table at the same time. Even negative and/or floating point numbers like -3.5 are valid table keys interestingly. Therefore my proposed syntax is not only usable for arrays and allows for that distinction.

-- Equivalent to table["2"]
Config.get("table/2/some/value")
-- Equivalent to table[2]
Config.get("table[2]/some/value")

As for the implementation of such a query syntax, the easiest way that comes to my mind would be to prefix anything in the form of [n] with a / so that a path some/array[1]/value would be split into the segments some, array, [1] and value.

In the methods each segment/key is first checked with string.find(key, "^%[-?%d*(%.%d+)?%]$") to check if it is an numeric key accessor and if true, will be casted using tonumber() before accessing the table value for the key.

for k = 1, #pathParts, 1 do
    if (type(part) ~= "table") then
        value = nil;
        break;
    end

    key = pathParts[key]
    if (string.find(key, "^%[-?%d*(%.%d+)?%]$") then
        key = tonumber(key)
    end

    value = part[key];
    part = part[key];
end

I have not actually tested that rewrite yet, it's just a simple idea on how to handle such a syntax extension. I would try to extend the config module from your pull request once it has been merged.

from factorio-stdlib.

Man this is a tricky one to work out how to do properly. Especially since Factorio handles the the table storage/loading for you (assuming you use the global var). Generally (in the other versions of this module for other languages) you were reading/writing the config from what was essentially a flat file database where everything lost it's original type and became a string when read back in. You had to specify getBoolean, getInt, etc to convert your data to the correct type otherwise you got a string version of your data. Each of those get* functions basically did return toTYPE(Config.get()). So because of that the Config module and the the incarnations i've seen/used for other languages assume that each of the keys in the path is a string key. In other words they only work with associative arrays/tables exclusively.

I've thought a lot about it for a while now on how to enable indexd tables and other fancy stuff. This is the best I could come up with that makes "sense" to me and would be easier to implrement without have to scan each and every string key for "magic" characters.
So how about this as a compromise that still does what you want as far as I know? I reserve the use of keys starting with [, { and ( for special use cases. So if your path is this/is[1]/a/test then is[1] is a string key but if your path was instead this/is/[1]/a/test you will get get seen as a "magic" key converting it to a numbered index. parenthesis will be used for "functions" like last. With { being for a future use of some sort.

That would allow for your desire to use things like this.

Config.get("table[2]/some/value")
-- Return array of values 1 through 3
Config.get("some/path/to/array[1-3]")
-- Return array of values 1 and 3 only
Config.get("some/path/to/array[1,3]")
-- Return last value in array
Config.get("some/path/to/array[last()]")

Only their form would change a little into

Config.get("table/[2]/some/value")
Config.get("some/path/to/array/[1-3]")
Config.get("some/path/to/array/[1,3]")
Config.get("some/path/to/array/(last)")

Config.get will throw an error if you do something like

Config.get("some/path/to/array/[bad]")
error("The key [bad] is not a valid key type")

What are your thoughts on this?

[edit]
Fixed a few mistakes in my wording. Probably more still not found.

from factorio-stdlib.

I feel like that restructure, while being functionally mostly identical, loses a lot in terms of intuitiveness. Function calls look a bit more like placeholder keywords in a template engine and what looks like a standard array access in most languages (array[1]) is actually a whole string key.

Also, it just relocates the issue of array[1] not being able to be used as a string key to [1] not being able to be used as a string key. There will always be some combination of characters that won't be allowed as string key anymore, to enable numeric key access for arrays. And the more fancy stuff is added to the query syntax, the more of those cases will exist.

I'm not saying the ideas are bad, it's just that the syntax does not resemble most known concepts of mine and therefore looks a bit weird for me. It would probably be beneficial to get some additional input from one or two of the other guys in here.

from factorio-stdlib.

Personally I'd just have index values as just normal path parts: "some/path/to/array/14/another/object/path"

Where 14 is just the index in the array. Doesn't lua treat them the same anyway?

Please excuse the quick reading if I missed something, on my phone. ^.^

from factorio-stdlib.

There will always be some combination of characters that won't be allowed as string key anymore, to enable numeric key access for arrays. And the more fancy stuff is added to the query syntax, the more of those cases will exist.

That is why I specifically want to reserve [ ], { }, and ( ) from the start. Now I'm thinking we should just go a head and reserve all of these from the string key paths? That way we can add features to the string key paths as needed. Would, imo, make the tests simpler too going forward as we wouldn't have to constantly check for backwards compatibility and/or knowingly break a certain string key path someone had been using but probably should have been anyways.

` ~ ! @ # $ % ^ & * ( ) + = [ ] { } \ | ; : ' " , < . > / ?

If we do reserve all of those. Then we could technically allow for both array[1] and array/[1]. Both behaving identically. Quick question though. What about this theoretical table.

local test = {array={{true}}}
local cfg = Config.new(test,{PathSeparator="/"});

--With your syntax do you reach that with this?
cfg.get("array[1][1]") --returns true
--If so I can't help but feel that would become a parsing nightmare for deeply nested index tables. I'm probably wrong on that but from your earlier example 
-- "^%[-?%d*(%.%d+)?%]$"  <- black magic to me. lol

--My syntax you just keep adding separators until you reach your desired depth.
cfg.get("array/[1]/[1]) --returns true

It would probably be beneficial to get some additional input from one or two of the other guys in here.

I'm sure they've poke their heads about this thread already. We've been going back and forth quite nicely slowly getting to workable extendable solution for this problem. So they are probably just keeping an eye on the discussion. :)
As I am rereading my post before posting @OvermindDL1 appears proving my point. lol

@OvermindDL1
This is a valid table

test = {
    "this has the index key of the number one"
}
test["1"] = "This has the string key of one"

Now both 1 and "1" co-exist in the table.
Plus how do we know if the Config user really did want a string key or a number key? My plan was to assume everything is a string key unless one of the reserved characters were used to say otherwise. Sure the syntax looks a little weird with "some/path/to/array/[14]/another/object/path" but it is consistent with how the paths are determined and lets the user tell the Config module exactly what they wanted at the same time.

But like I said above by reserving the list of characters we could possibly allow both forms.

from factorio-stdlib.

Ah different types, true yeah.

from factorio-stdlib.

You just keep adding and removing ingredients until you find out how to make gold 😄

I can't even do that. I always get lead. lol

Concerning the syntax parsing, I think you overlooked a sentence of mine. Basically, because of intuitiveness, I want the syntax for the user to be some/array[1][1], but internally we would still handle some/array/[1]/[1|, which is of course easier to parse.

I think I did. We ended up on the same page in the end though. That was that was how I intended code in support for that syntax.

Overall I'd suggest to draw inspiration form JSONPath which is a lot nearer to how lua tables store data than my first suggestion, XML itself.

That looks like the kinda power I'd get into trouble with. Let's do it. :D
I'll reserve the character list from above so that users can do simple things like this without issue but power users can do the fancy stuff too!

--Simple
Config.get("simple/getter/for/my/data")

--Powerful
Config.get("some/array[:5]")

from factorio-stdlib.

Sounds good. The nice thing with the reserved characters is, that for now you can push the Config module with the "simple" way of accessing data already working and we can just build on top of that.

Concerning the possible functions, in retrospect I'm not sure there is a need for the length() one as the following two lines of code do exactly the same with the latter being less verbose.

Config.get("some/array/length()")
#Config.get("some/array")

The implementation of the min(), max() and avg() functions on the other hand could probably be put into the table module, so that an occurrence of those in the path would issue something like:

table.min(array)
table.max(array)
table.avg(array)

from factorio-stdlib.

Question on separators. Shall I restrict them to a single character in length? Would make things easier for everyone if no one had to worry about people using crazy multi-character separators just because they could.
Then on the same thought line also limit them to . / space-character as the only valid separators?

My votes is, obviously, yes on both parts. Otherwise we'd have parsing headaches if someone used [ for a separator. :/

from factorio-stdlib.

I'd agree to both, too. On the other hand, is there actually a real benefit to having the path separator be user definable? (Especially if we'd limit the possible characters so much?) If not, maybe the simplest option would be to just settle on one.

Though I am not sure which one would be superior. / seems a bit clearer, while . is directly connected to the way tables can normally be accessed in lua. Spaces, I think, look just weird and more like a sentence than a path.

from factorio-stdlib.

We can strike off the space character then. That leaves / and . which are harder to decide upon. I had the option to choose your separator for the exact reasons you mentioned. Allowing anything was just a side-effect of not intentionally restricting it.

I prefer / but I can change to . just as easy. So it really makes no difference to me.

How about allowing both as the separator at the same time? Easy enough to replace all of one with the other before splitting the string into pieces. Then make note in the docs
'/' and '.' are both used as path separators.

[edit]
I just discovered that there is already a Lua implementation of JsonPath. No idea why I never tried looking before now. I think the best...OK, easiest...option now is for the Config module is to be a wrapper for the Lua JsonPath or modify it to work on the table given to it when constructed. Otherwise I'm just reinventing the wheel.

@Afforess Need input on how feasible that is license wise. Creating a stdlib-ext for special modules that have a license problem or undesired in the main stdlib is always a possibility I would assume.

[edit2]
I say we treat all strings that do not start with $ (to indicate a JsonPath query) as simple path request. I've already started making the Config module behave this way.

[edit3]

• Struck out the top of the post as I finalized . as the only separator.
• Clarified what I meant in edit2.

from factorio-stdlib.

Do you have a link? I've only found ones depending on an external dependency, that I'm not sure will be possible to also include.

Also, I'm not sure it would be the best idea to just wrap it around such a library, because in my experience it hardly ever works out great, when "misusing" a library for a purpose it was not fully designed for. Sure, we use JSONPath as basis for our syntax, but there are specific features we'd probably leave out (like starting queries with $) to not overcomplicate the whole matter.

from factorio-stdlib.

https://github.com/mrpace2/lua-jsonpath which depends on LPEG

I also literally finished a overhaul on the Config module. Take a look at the PR #33

This is the most recent changelog if you don't want to look at the code. :)

Overhauled Config Module

• + Forbid non-alphanumeric characters in SimplePaths
• + Only separator allowed is now a period
• + Initial detection for deciding if path is a JsonPath or a SimplePath
• - Removed Options table

Overhauls Config_spec

• + Existing tests are now sane.
• + Fixed coverage on changed features
• + Added coverage to new features
• + Added coverage to a few missing use cases

from factorio-stdlib.

It would probably be beneficial to get some additional input from one or two of the other guys in here.

I've been following but don't have much stake in this particular effort. I don't usually use much configuration.

External libraries are fine to include as dependencies, but be careful as a lot of lua libraries have external C dependencies which make them useless for us, as we are in a sandbox and can not compile them.

Specifically, LPEG looks like it is non-pure, but this claims to be a pure drop-in replacement: https://github.com/pygy/LuLPeg/blob/master/README.md

Edit:
From a licensing perspective, all the discussed libraries seem to be MIT, which is the same license as stdlib, so I don't see that being a problem. The only major issues are going to be using dependencies with non-free licenses or viral licenses like GPL/AGPL. Every other FOSS license should be ok.

from factorio-stdlib.

Specifically, LPEG looks like it is non-pure, but this claims to be a pure drop-in replacement: https://github.com/pygy/LuLPeg/blob/master/README.md

I'm glad you found that. I was worried last night after I discovered LPEG had a C dependency right before going to bed.

Since you think their inclusion would be ok. Should I make a stdlib-dep folder and put extra external dependencies in there? I can then pcall require to test for them then fallback to an error message if missing.
error("Lua-jsonpath and LuLPeg must be in stdlib-dep to use JsonPath extensions to Config")

from factorio-stdlib.

Since you think their inclusion would be ok. Should I make a stdlib-dep folder and put extra external dependencies in there? I can then pcall require to test for them then fallback to an error message if missing.

Sounds like a decent plan.

from factorio-stdlib.

Finally got some time to work on this today. Getting LuLPeg and JsonPath working inside Config was surprisingly simple and straight-forward. For some reason I did not expect that. Also, getting 4 Config functions working with JsonPath support was scary easy, too. lol.

With "SimpleQueries" everything works the same. Starting a path with $ will hand off to JsonPath for processing. This is the current setup I have for the 4 Config functions.

• .get
  • SimpleQueries: Returns the direct value of the query or nil if nothing was found.
  • JsonPath: Returns the table from JsonPath or nil if the table was empty.
• .set
  • SimpleQuery Always returns true. It brute forces your path to exist.
  • JsonPath Works more like a mass updater than a true set. Don't see a better way to handle this one. Returns the number of "set" values or false if JsonPath table was empty.
• .delete
  • SimpleQuery Returns true if the path was delete or false if it wasn't found.
  • JsonPath Returns number of deleted entries or false if JsonPath table was empty.
• .isset
  • SimpleQuery will continue to return a plain true/false
  • JsonPath will return the number of "trues" (really just length of node table) or false if JsonPath returned nothing.

This sound good to everyone?

from factorio-stdlib.

is_set please. snake_case is the Factorio style and we should stick by it.

from factorio-stdlib.

Done. I never thought about that. So obvious once you said it.

[edit]
I'll push the JsonPath changes, shortly once I verify the tests do indeed cover enough for JsonPath. Everyone can play with it and see what they think. If everyone is satisfied then I say we are ok to pull.

from factorio-stdlib.

It looks okay, although it's not exactly what I had in mind. The reason you had for splitting the querying into SimpleQuery and JSONPath is one of the reasons I would have opted for a custom implementation of a subset of JSONPath's functionality. Like I said above, trying to fit some library into something it wasn't designed for rarely is beneficial in my experience.

Another small issue/inconvenience I've come across while studying the library: it's indices in the syntax are 0-based while the processed and returned tables still have 1-based indices, as is normal in lua. I can see that causing a lot of confusing errors for users.

On a more constructive note: Wouldn't it be simpler to merge SimpleQuery and JSONPath? A SimpleQuery with a $. prefixed to it would be a valid JSONPath, wouldn't it? In that case it would seem simpler to just let users use paths in the form of abc.def.cfg(with or without JSONPath capabilities) and internally prefix the $. to use the library for all cases.

For get there wouldn't be any changes to the way it returns data, while the other three would change a bit. set and delete would always return the number of affected values, 0 would be equivalent to a SimpleQuery false and 1 to a true return value. In the case of isset I would favor returning true if all affected values of the path are set, and otherwise returning false.

from factorio-stdlib.

Another small issue/inconvenience I've come across while studying the library: it's indices in the syntax are 0-based while the processed and returned tables still have 1-based indices, as is normal in lua.

...I didn't even notice. I'll poke about JsonPath tomorrow and fix it. Shouldn't be to awful to do.

On a more constructive note: Wouldn't it be simpler to merge SimpleQuery and JSONPath? A SimpleQuery with a $. prefixed to it would be a valid JSONPath, wouldn't it?

I actually erased that from my previous post before committing it. Didn't want to unintentionally persuade anyone by mentioning it. Yes, at this point switching to a full on JsonPath would be easier and make for better consistency all-round. My preferred option, however, is to break Config in to two parts.

--The module as it existed pre-jsonpath. Plain, simple, works.
stdlib/config/simple.lua

--[[The JsonPath only module. Doing this would make this file only
    add write support to what would otherwise be a read-only
    library. (not a bad thing, imo)
]]--
stdlib/config/jsonpath.lua

This opens the door to other query styles if people wanted to add them. Such as SQL query.

while the other three would change a bit. set and delete would always return the number of affected values, 0 would be equivalent to a SimpleQuery false and 1 to a true return value. In the case of isset I would favor returning true if all affected values of the path are set, and otherwise returning false.

I internally debated for a while and went with trying to keep close to the SimpleQuery way. That said I think switching always returning the affect row count is fine.

As for is_set the way JsonPath works it only returns what is set. So unless JsonPath return a zero length table is_set will always return true. So I went with the count of returns results from JsonPath. Then let the lib user decide if that count was high enough or not.

It is also for this reason I mentioned JsonPath set acts like a "update" because JsonPath doesn't return non-existent values.

Another thing JsonPath doesn't do is return references to tables. To actually add set and delete support for JsonPath queries by cheating. I concat the paths returned from JsonPath to perform a SimpleQuery to do the real set/delete operation.

from factorio-stdlib.

So before I head into work I decided to split Config module into Simple and JsonPath. Getting them both split up was easy enough but the issue I mention before with jsonPath .set really just being a update function is really really obvious now. I'll play around with it more later.

I read what you were saying before but didn't fully comprehend it until now.

The reason you had for splitting the querying into SimpleQuery and JSONPath is one of the reasons I would have opted for a custom implementation of a subset of JSONPath's functionality. Like I said above, trying to fit some library into something it wasn't designed for rarely is beneficial in my experience.

I had to learn that one that hard way, I suppose. Now that I've gotten somewhat intimate with JsonPath. For .get, .delete and isset they work just fine with the full feature set of JsonPath. .set however is used for creating new table entries and updating existing ones. No matter what feature set we take from JsonPath it will still only 'see' existing table entries. This poses the biggest obstacle to my knowledge. For the split up config modules .set is the only one failing the tests. I can't even get it to insert an easy entry like .set("z",12345) Since the search return no existing records. I'd have to pull out and modify a lot of JsonPath just to put it into the .set function only.

Unless I can think of some magic solution at work I'm out of ideas to make this work with any portion of JsonPath's features for .set

from factorio-stdlib.

I think we just jumped the gun a bit going from simple numeric key access, to XML/JSONPath inspired add-ons to just completely including JSONPath. What I would suggest is, that you - for now - create just the simple Config module with some.path.to.something path syntax and with the aforementioned characters reserved for future use.

Once we have that going, we can look into the whole thing with a set of fresh eyes. It would be our minimal viable product from which we then can work off to maybe extend the possibilities a bit here and there. As I said before, I'm happy to work on the parsing, as I have some previous experience with stuff like that and regular expressions. And I already have some ideas on how to handle a more complex syntax.

A first step would be to add in simple numeric key acces with the some.array[1] syntax. That is not to hard and as I showed somewhere above, can be done with only a few additional lines of code.

A second step could be to include some functions like min() and last(). Again, at that point, we have an already working module, which we just need to refactor without breaking the existing tests.

In my opinion and experience that approach would be simpler, safer/more stable and, if done properly, faster.

from factorio-stdlib.

Yeah, I got over excited when I saw what JsonPath could do and realized I could integrate it into the module easily. Completely over looking the gotchas until they were pointing and laughing at me.

I'll make two new branches on my github. One for the plain jane config module that I'll open a PR for in a day or two. Give it time to settle. Then a 2nd branch as a holder branch for the current mostly working JsonPath version of the config module. That should ease pulling over anything we need from there to wherever.

Shall I make the simple module return return 1/0 instead of true/false so when support for multiple return values is coded in it remains consistent?

from factorio-stdlib.

I think that is the way to go. Maybe we'll use it, maybe we won't, but in any case it's future proof and won't cause possible breaking changes to the interface. Returning different types of data depending on the query (single value vs multiple values) is not a good design in my book as it can lead to confusion.

from factorio-stdlib.

Here is the split off simple config module. configv2
The mostly working jsonpath version will be at configv2-jsonpath once I push it in a bit. Needs just a bit more clean up work from this morning before I push it. Only because it will bother me if I don't do it now. Even if it never gets touched again. lol

from factorio-stdlib.

Been a few days now. Pushing a PR for configv2 branch inclusion into master.

from factorio-stdlib.