Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for lexicographical comparison for numeric operators #14

Open
sarensw opened this issue Mar 11, 2020 · 3 comments
Open

Support for lexicographical comparison for numeric operators #14

sarensw opened this issue Mar 11, 2020 · 3 comments

Comments

@sarensw
Copy link

sarensw commented Mar 11, 2020

I want to use the < and > operators to compare strings lexicographically (Dates in form of yyyy-MM-dd to be more precisely). This works in JS but unfortunately not in this .Net port. It throws a FormatException because the strings cannot be converted to double.

I forked and tried to fix this but I'm having a hard time doing so due to the Func based nature in the code.

As a first try, I have changed DoubleArgsSatisfy so that it works in a generic way. But I'm still only able to set one type when calling this method in AddOperator

AddOperator(">", ArgsSatisfy<string>((prev, next) => string.CompareOrdinal(prev, next) > 0));

private Func<IProcessJsonLogic, JToken[], object, object> ArgsSatisfy<T>(Func<T, T, bool> criteria)
{
    return (p, args, data) => {
        var valuesString = args.Select(a => a == null ? "" : p.Apply(a, data).ToString()).ToArray();
        var values = valuesString.Select(v => (T) Convert.ChangeType(v, typeof(T))).ToArray();
        for (int i = 1; i < values.Length; i++) {
            if (!criteria(values[i-1], values[i])) return false;
        }
        return true;
    };
}

Can you give me a hint on how to improve this?

@sarensw
Copy link
Author

sarensw commented Mar 13, 2020

I have created a pull request with a proposed solution for this. @yavuztor could you please have a look at whether this one is ok for you?

@yavuztor
Copy link
Owner

yavuztor commented Mar 21, 2020

I have created a pull request with a proposed solution for this. @yavuztor could you please have a look at whether this one is ok for you?
@sarensw , the fix in the pull request is good, but could be easier to read with some refactoring. Can you check my comments to see if they make sense?

Also, I pushed some changes to work around the missing signing key issue on build. It should build fine next time.

@FrederikLynggaard
Copy link
Contributor

Hi,
I have created a refactored pull request fixing this issue (#26).
It is based on @sarensw's implementation.

@yavuztor, could you have a look at it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants