factoriolib / flib

A set of high-quality, commonly-used utilities for creating Factorio mods.
https://mods.factorio.com/mod/flib
MIT License
64 stars 15 forks source link

Add a misc function to intelligently split a string #17

Closed ClaudeMetz closed 4 years ago

ClaudeMetz commented 4 years ago

This adds a small misc function to split a string. It works the same as the one in the vanilla lualib, except it also converts any numeric substrings to be of type number, using tonumber(). This is much more convenient to use when some of your substrings are numbers, like numeric ID's or a part of a version string, for example.

If there's anything wrong with this submission, please let me know. I've not submitted code to any real project before, so I might be doing stuff incorrectly here.

ClaudeMetz commented 4 years ago

I guess you could also replace token = number_token or token; table.insert(split_string, token) with table.insert(split_string, (number_token or token)) ... or just make it table.insert(split_string, (tonumber(token) or token))

0ptera commented 4 years ago

I'm generally against bloating Flib with functions that are faster to type (and execute) than a library function call.

ClaudeMetz commented 4 years ago

It already is a library call though? You need the function to split a string anyways. This just saves you from having to postprocess that data yourself afterwards every time.

0ptera commented 4 years ago

Since you mention postprocessing, that's another reason to not add specialized tiny functions to a library.

Instead of

local split_string = flib_misc.split_string(string, separator) 

directly use

local split_string = {}
for token in string.gmatch(string, "[^" .. separator .. "]+") do
  split_string[#split_string+1] = (split_string, (tonumber(token) or token))
end

This way you can directly treat each substring differently as required.

ClaudeMetz commented 4 years ago

Don't understand the postprocessing remark, what do you mean by that?

Regarding your example: I'm not sure if I'm off base here, but isn't the point of a library to have functions that avoid you having to duplicate code everywhere? If I want to split a string in 10 places in my mod, do I have to write that out every time? Isn't that the exact opposite of what you're supposed to do? I don't feel like it is trivial enough for that.

What it should really be is part of lua itself, be it the number-converting version or just the basic one. But again, shouldn't a library fill in holes that the language doesn't? Also, there isn't any different processing that can happen, it's only the thing with numbers being actually returned as numbers, and not strings. No different treatment of the substrings happens.

I would maybe get the argument that you think this is too niche (which I think it isn't), but saying it's not worth to modularize is what I don't get.

JanSharp commented 4 years ago

A library function should only do one thing which the name and the short description would already describe to it's fullest. If you manage to do that, good job. Sometimes that is difficult, just look at the gui module. It is almost impossible to build an entire module and have each function be self explanatory.

This split function however is not part of any complex module. It also does multiple things, one of which isn't even splitting a string, it's converting the type of values. It is near impossible to tell what this function really does, unless you read the source code.

I personally would find it better to have a function that just splits the string, taking the entire pattern as an argument to keep it easily readable, and then convert the strings to numbers when they are used, to keep that part clear as well.

If lua doesn't have a split function, then that might make sense to add, but splitting strings is a difficult task to handle generically.

raiguard commented 4 years ago

Lua does not have a string.split function, so I'm in support of this. It's always something that I end up adding to my mods and it would be nice to have it as a flib function.

0ptera commented 4 years ago

lua has gmatch and match.

Like I said before, all this proposed function does is wrap gmatch into a rigid frame. The only potential use for this I can think of, would be version checking. However I did the benchmarks, using string format to add leading zeros and compare versions as one string each is faster and easier to use.

raiguard commented 4 years ago

After more thought, I agree that automatically coverting to numbers would not be a good thing to do. It's not intuitive and would need explaining. Not that I can talk, the GUI module is quite complicated... But that's more of a framework than a library function.

The only thing I don't like with using gmatch directly is the boilerplate of the pattern. But, making that into a callable flib thing would end up being more typing in the long run. So now I'm unsure.

ClaudeMetz commented 4 years ago

The base game lualib already has string splitting, like I said, so it's not necessary to add that to flib.

I use this function because, in the past, I looked for a bug for a hot minute before realising that the thing I expected to be a number is really a string. Still, not a good fit for flib. I still disagree with Optera though that this should not be a function at all, nearly every scripting language has that in its standard library, for good reason imo.

This can be closed, if no-one else has input on this. Thanks for considering it.

raiguard commented 4 years ago

string.split() is something that is in the standard library of almost every other scripting language in existence, so I don't think it's "extra fluff" or "bloat" at all. While you can achieve the same thing with gmatch, it's a lot less convenient and a lot more annoying.

I do believe that automatically converting to numbers where possible is a bad idea, so if this makes it in, that specific feature won't.

0ptera commented 4 years ago

Without converting to numbers the whole request is even more of a pointless wrapper for gmatch. Do local string.split = string.gmatch if you feel like it needs the name split.

ClaudeMetz commented 4 years ago

Yes, if you don't want to convert numbers, don't add this, because as I said, the base game lualib already has a function to split a string.