Add functions to check Range, IP and CIDR. Included test and docs.

[kemasdimas]

I accidentally closed #59 while cleaning up my forked repo 😓. All the commits have been squashed into a single commit for now.

[kemasdimas]

IPv6 Consideration

My mind can't stop thinking about IPv6 implementation, so it's better to write it here 😊

Looking back to the eval code

fun eval(formula: String, env: JsonObject): Float {
    try {
        return when (val result = RuleGrammar.parseToEnd(formula).eval(env)) {
            is Boolean -> if (result) 1f else 0f
            is Number -> result.toFloat()
            is String -> result.runCatching { toFloat() }.recoverCatching { 0f }.getOrThrow()
            else -> 0f
        }
...
}

If it was designed to convert any result to Float which is then be used in the isEnabled function.

I think we should reconsider using a separate function to process both IPv4 and IPv6 in 1 function, and this new function will return either 1f or 0f

Why?

Converting IPv6 can't be done with a single variable, but in this case, using an array is not possible either. Because the eval will always return Float.

And currently, I can't find a way to accurately represent 2 Long variables into 1 Float 😁

So adding IPv6 support in the current ip function seems to be incompatible with eval

Proposal

We can create one function to support both IPv4 and IPv6, i.e ipmatches("2001:4860:4860::8888/32", "2001:4860:4860::8888")

I know... we'll be back to the first proposal (creating a separate function just for IP)😅

But, this single function will accept both IP formats, without needing to revamp the whole eval concept

[goncalossilva]

About the PR itself, I left a couple of follow-ups on #59 since they were inline. Please have a look.

In general, I think we could merge IPv4 support and continue discussing IPv6 in a new ticket, but I'm fine to combine both together if you feel that's best. I'm enjoying thinking this through and ensuring we have a great design and implementation. Thanks for the back-and-forth!

If it was designed to convert any result to Float which is then be used in the isEnabled function.

Something to note about eval is that it's the last operation of all. There are intermediate representations of data (string, boolean, number, array, etc.) and that's OK. In fact, many functions expect to work with certain data structures that are not floats.

It's also worth noting that the existing functions -- eq, lt (+ all others), contains -- don't make many assumptions. As long as something understands equality and implements Comparable, it should work. For example, if ipv6 returned an internal class backed by 2 longs that implements Comparable and equals/hashCode, and cidrv6 returned a range between 2 of those instances, the rest should mostly work out of the box.

We could also look into supporting both formats under ip and cidr. I don't think it would require many changes, but to be fair, I haven't prototyped it.

And currently, I can't find a way to accurately represent 2 Long variables into 1 Float 😁

It's also impossible to represent a single Long in a Float, since a few bits go into exponent precision. The max value is around 2^53, not 2^64! But hopefully, it's clear why that's not a problem -- converting to Float is the very last step, and we're actually just looking to get it within the [0,1] range. 😊

[kemasdimas]

I will address the comments in https://github.com/Doist/ffs/pull/59#discussion_r796191947 to finalize IPv4 implementation

And then I'll open a new issue on IPv6 to keep the discussion relevant 😊

[kemasdimas]

There's also a failing test case:

assertEquals(3232238083f, eval("""ip("192.168.10.3")"""))

Interesting... this test case was passing when I ran it from IntelliJ (by running the commonTest folder)

If you have alternative ways to run the test suites, please let me know. Would like to make sure that the implementation is bug free 😊

[goncalossilva]

The way CI runs tests is here: https://github.com/Doist/ffs/blob/9b1e61c9538708831155d3ea06efd70e0d8519a9/.github/workflows/ci.yml#L21

I see the failing test in the checks of this PR. Don't you?

[kemasdimas]

The way CI runs tests is here:

https://github.com/Doist/ffs/blob/9b1e61c9538708831155d3ea06efd70e0d8519a9/.github/workflows/ci.yml#L21

I see the failing test in the checks of this PR. Don't you?

Wow, this one got me good! 😅 never should've carelessly converted Long to Float in the first place, let alone created a test for that. All failed tests came from the JS platform, I'll make sure to check tests on all platforms before committing

I should've checked this beforehand

It's also impossible to represent a single Long in a Float, since a few bits go into exponent precision. The max value is around 2^53, not 2^64!

Thank you for pointing this out!

[goncalossilva]

All failed tests came from the JS platform, I'll make sure to check tests on all platforms before committing

Ah, yes, JS is tricky. The entire reason castEval exists is because of JS's leniency with types.

[goncalossilva]

CI is green, we're good to go! There are a few tiny changes I'd like to make, but I'll do them right after. The PR is gold, thanks for contributing!

[kemasdimas]

You're welcome, thanks for all the time you've set aside to review my pull request!

I've opened new issue #65 to start implementing IPv6