framework icon indicating copy to clipboard operation
framework copied to clipboard

request: easier syntax for combining preconditions

Open favna opened this issue 3 years ago • 7 comments
trafficstars

Is your feature request related to a problem? Please describe.

Currently the way to combine preconditions is using nested arrays such as: [['AdminOnly', ['ModOnly', 'OwnerOnly']], 'InVoiceChannel'];. This is extremely developer unfriendly because it's extremely chaotic and you have to thoroughly know how preconditions are parsed to start making the least amount of sense of this array.

Describe the solution you'd like

We can take a note out of how other libraries handle cases like this, such as Prisma who has $and, $or, and $not to better build up the preconditions list.

Describe alternatives you've considered

Using what we have no? Nah.

Additional context

https://github.com/sapphiredev/website/pull/91/files/a56d8a9c7f2ac760ea394241092fcdcbc583afa8#diff-7cc7d069f52b3fcc7bf7039c05a4988bd8f895d16731450c21c391e1d0c06565

favna avatar Nov 29 '21 20:11 favna

Wasn't using a builder class also considered a while ago?

Lioness100 avatar Nov 29 '21 20:11 Lioness100

I don't recall it but I can see it having some merit as well.

favna avatar Nov 29 '21 22:11 favna

Wait also can't you do { mode: PreconditionRunMode.Or, entries: [a, b] } and stuff

Lioness100 avatar Nov 30 '21 00:11 Lioness100

Why not have a static class WhateverPreconditions and have methods and, or and not just like prisma

UndiedGamer avatar Jun 26 '22 14:06 UndiedGamer

I'd think a helper class would be easy enough, it would only be something like this:

type PreconditionString = /* something */ string;
type RecursivePreconditionStrings = (PreconditionString | RecursivePreconditionStrings)[];

class PreconditionList extends null {
    public static and(...entries: RecursivePreconditionStrings) {
        return entries;
    }

    public static or(...entries: RecursivePreconditionStrings) {
        return entries;
    }
}

// ["0", ["1", "2", ["3", "4"]]]
PreconditionList.and("0", PreconditionList.or("1", "2", PreconditionList.and("3", "4")));

I guess the problem with that is there's nothing stopping a user from doing two of the same (.and/.or) in a row, which would give unexpected results. However, that wouldn't be able to happen if we used an object: (excuse the messy types)

type PreconditionString = /* something */ string;
type RecursivePreconditionList = (PreconditionString | { $or?: (PreconditionString | { $and?: RecursivePreconditionList })[] })[];
type Preconditions = (string | Preconditions)[];

const resolvePreconditionList = (list: RecursivePreconditionList): Preconditions => {
    return list.map((element) => {
        if (typeof element === 'string') {
            return element;
        }

        if (Array.isArray(element)) {
            return resolvePreconditionList(element);
        }

        return resolvePreconditionList(Object.values(element)[0] as RecursivePreconditionList);
    })
}

// ["0", ["1", ["2", ["3", "4"]]]]
resolvePreconditionList(['0', { $or: ['1', { $and: ['2', { $or: ['3', '4'] }] }] }])

The object design is also less verbose, so I think that's the best way to go. Thoughts?

Lioness100 avatar Nov 06 '22 14:11 Lioness100

The idea is interesting. Instead of using a class extending null we can also leverage TS namespaces:

export namespace PreconditionList {
    export function and(...entries: RecursivePreconditionStrings) {
        return entries;
    }

    export function or(...entries: RecursivePreconditionStrings) {
        return entries;
    }
}

I prefer this over an object tbh. Sure an object may be typing less but verbosity adds clarity as well.

favna avatar Nov 06 '22 20:11 favna

I understand where you're coming from, but honestly in this case, I don't think a leading PreconditionList.* (or whatever we'd call it) would add any clarity. People would already know it's a precondition list by seeing preconditions: [...]. And i don't know how the $and and $or could be misconstrued out of the intended context.

However, if we do go with functions, I agree namespaces (or just plain objects) would be better.

Lioness100 avatar Nov 06 '22 21:11 Lioness100