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

request: easier syntax for combining preconditions #334

Open
favna opened this issue Nov 29, 2021 · 7 comments
Open

request: easier syntax for combining preconditions #334

favna opened this issue Nov 29, 2021 · 7 comments
Labels
Milestone

Comments

@favna
Copy link
Member

favna commented Nov 29, 2021

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

@Lioness100
Copy link
Contributor

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

@favna
Copy link
Member Author

favna commented Nov 29, 2021

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

@Lioness100
Copy link
Contributor

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

@UndiedGamer
Copy link
Contributor

UndiedGamer commented Jun 26, 2022

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

@favna favna added the feature label Jul 24, 2022
@favna favna added this to the v4 milestone Jul 24, 2022
@Lioness100
Copy link
Contributor

Lioness100 commented Nov 6, 2022

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?

@favna
Copy link
Member Author

favna commented Nov 6, 2022

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.

@Lioness100
Copy link
Contributor

Lioness100 commented Nov 6, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Todo
Development

No branches or pull requests

3 participants