simplify the elsif-chain in is_perl_supported - #274

for readability I prefer the longer version personally.

for readability I prefer the longer version personally.

Yeah I think I find the longer version more readable, too, actually...

Fair enough, feel free to close then.

I considered using the /x regex flag for introducing extra whitespace, so that the code reads more similarly to the old code; but I don't know if we're guaranteed to have a new enough Perl for that. Also considered using a hash to store the set of valid versions, but went with the regex instead. If either of these approaches would be preferred, I can change the PR to do those instead; otherwise, feel free to close

I can't find what version of Perl started supporting /x. /xx came out in perl-5.26, so obv before that. And I saw forum comments from 2018 referencing /x, so around 10.14 maybe. That still leaves us with some years of supported perls (5.16 and 5.18) to worry about. W/out making a new commit, what would the hash look like ? (not a perl person)

I also agree that this is less legible. A hash could work (checking for existance of the value as a key), just need an efficient way to construct it (using a loop would make this less-efficient overall, though not that it is often-used often enough to matter).

I can't find what version of Perl started supporting /x. /xx came out in perl-5.26, so obv before that. And I saw forum comments from 2018 referencing /x, so around 10.14 maybe. That still leaves us with some years of supported perls (5.16 and 5.18) to worry about. W/out making a new commit, what would the hash look like ? (not a perl person)

Yeah, I couldn't seem to find any info on when exactly the /x flag was added. I do know it's been around for quite a while (though I do also seem to recall there having been some bugginess in the earliest versions). It's been decades since I've looked at Perl, so it's hard to recall that sort of detail :)

The hash version would look something like:

my @versions = (
    "5.foo" => 1, 
    "5.bar" => 1,
    ...);
return $versions{$]};

All those "=>1" make it more verbose than the regex version, though I still feel like it's marginally cleaner than the current elsif chain.

I also agree that this is less legible. A hash could work (checking for existance of the value as a key), just need an efficient way to construct it (using a loop would make this less-efficient overall, though not that it is often-used often enough to matter).

If performance is critical, then the regex version will be the fastest: all perl needs to do is parse the regex and apply it. (The ugliness of the current PR is because I've already done the regex optimization. Or rather, most of it: I left some shared prefixes, because even I couldn't stomach what it'd look like to combine them ;)

For the hash version, I'd just give it as a literal, both for performance and for legibility (IMO having all the redundant "=>1" is more legible than using a loop to convert an array into a hash). To avoid the cost of redundantly constructing the hash, we could always define it globally rather than locally; though that has its own legibility and maintenance concerns, so I think it's best avoided unless we can demonstrate that this function really is a bottleneck/hotspot.

I don't think there is any argument that regex would be faster, but we need someplace in the middle. Otherwise future released will just make this harder and harder to add to and make it much much easier to mess it up.

I don't believe a small overhead will make or break fink so readability and future updates will have a higher weight then pure speed here.

Yeahno, just to be clear, my only concern here is about legibility and maintainability :)

Although the elsif chain does have some performance issues (since it's essentially doing a linear scan through a list), fixing that is just a nice side benefit. Honestly, I'd be just as happy with an implementation like return List::Utils::any { $] eq $_ } "5.foo", "5.bar",...; —since it still achieves the main goal of removing the repetitive "if ($] eq ...)" code.