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

Union for variadic types #183

Closed
nzour opened this issue Apr 26, 2021 · 11 comments · Fixed by #185 or #184
Closed

Union for variadic types #183

nzour opened this issue Apr 26, 2021 · 11 comments · Fixed by #185 or #184
Assignees
Labels
Priority: Medium This issue may be useful, and needs some attention. Status: Accepted It's clear what the subject of the issue is about, and what the resolution should be. Status: In Progress This issue is being worked on, and has someone assigned. Type: Enhancement Most issues will probably ask for additions or changes.
Milestone

Comments

@nzour
Copy link
Contributor

nzour commented Apr 26, 2021

There are a lot of situations may occur when we need to define union from several cases.
For example: need to define enum of states.

This would look like:

use Psl\Type as T;

$codec = T\shape([
    // ... rest of fields
    'state' => T\union(
        T\literal_scalar('NEW'),
        T\union(
            T\literal_scalar('APPROVED'),
            T\union(
                T\literal_scalar('REJECTED'),
                T\union(
                    T\literal_scalar('COMPLETED'),
                    T\union(
                        T\literal_scalar('FROZEN'),
                        T\literal_scalar('ERROR')
                    )
                )
            )
        )
    ),
]);

Inferred type would be:

array{id: string, state: "APPROVED"|"COMPLETED"|"ERROR"|"FROZEN"|"NEW"|"REJECTED"}

I guess it would be great to have better solution like:

use Psl\Type as T;

$codec = T\shape([
    'id' => T\string(),
    'state' => T\union_of(
        T\literal_scalar('NEW'),
        T\literal_scalar('APPROVED'),
        T\literal_scalar('REJECTED'),
        T\literal_scalar('COMPLETED'),
        T\literal_scalar('FROZEN'),
        T\literal_scalar('ERROR'),
    ),
]);

I do understand that Psl\Type\Internal\UnionType has only 2 branches: left and right
That's why propose just implement a function with signature:

/**
 * @template T
 *
 * @param TypeInterface<T> $left_type
 * @param TypeInterface<T> $right_type
 * @param TypeInterface<T> ...$others
 * @return TypeInterface<T>
 *
 * @no-named-arguments
 */
function union_of(
    TypeInterface $left_type,
    TypeInterface $right_type,
    TypeInterface ...$others
): TypeInterface { // actually Psl\Type\Internal\UnionType returns here
    ...
}

As alternatives I may guess there is more suitable name for function.
This may be named like literals or one_of or etc.

This function may also be implemented in user land, but I think it would be great to have unified solution inside core of the library.


Want to hear other guys thoughts.
If this would be approved, I may produce PR.
As I can see, this is very light enhancement.

@nzour nzour added the Type: Enhancement Most issues will probably ask for additions or changes. label Apr 26, 2021
@azjezz
Copy link
Owner

azjezz commented Apr 26, 2021

Hm, i think we can allowing extra arguments to Type\union and Type\intersection ( i prefer if these two provide the same API ).

This will also prevent a BC break, in v2.0 we can drop $right and $left and make both union and intersection accept variadic arguments.

@azjezz azjezz added Priority: Medium This issue may be useful, and needs some attention. Status: Available No one has claimed responsibility for resolving this issue. labels Apr 26, 2021
@azjezz azjezz added this to the 1.7.0 milestone Apr 26, 2021
@nzour
Copy link
Contributor Author

nzour commented Apr 26, 2021

🤔 Hmm, I kept in my that UnionType has left and right branches, therefore function union() takes two arguments to be honest with users.

I was thinking about suggestion to define extra function to prevent boilerplate when we need define union type with a lot of cases.
But, If you think it is alright to expand already existed function - it is fair enough for me

@azjezz
Copy link
Owner

azjezz commented Apr 26, 2021

we can have function union(TypeInterface $right, TypeInterface $left, TypeInterface ...$other), UnionType is an internal class, there's no reason to design the public API to match it.

@nzour
Copy link
Contributor Author

nzour commented Apr 26, 2021

Looks great for me.

@azjezz
Copy link
Owner

azjezz commented Apr 26, 2021

Are you up for a PR?

@nzour
Copy link
Contributor Author

nzour commented Apr 26, 2021

Are you up for a PR?

I'm on it

@nzour
Copy link
Contributor Author

nzour commented Apr 26, 2021

By the way, why you want to make union and intersection completely variadic (in v2.0)?
union(string()) - doesn't make sense for me

@azjezz
Copy link
Owner

azjezz commented Apr 26, 2021

union(string()) - doesn't make sense for me

it doesn't to me too, but that's solvable via Psl\invariant(Iter\count($types) >= 2, 'at least 2 types are required to create a union.');

@azjezz azjezz added Status: Accepted It's clear what the subject of the issue is about, and what the resolution should be. Status: In Progress This issue is being worked on, and has someone assigned. and removed Status: Available No one has claimed responsibility for resolving this issue. labels Apr 26, 2021
@azjezz azjezz modified the milestones: 1.7.0, 2.0.0 Apr 26, 2021
@nzour
Copy link
Contributor Author

nzour commented Apr 26, 2021

union(string()) - doesn't make sense for me

it doesn't to me too, but that's solvable via Psl\invariant(Iter\count($types) >= 2, 'at least 2 types are required to create a union.');

Do you prefer runtime exception instead of statically analysed issue?

@azjezz
Copy link
Owner

azjezz commented Apr 26, 2021

the reason is i don't like $right, $left, $others, maybe we could just rename them to something more appropriate...

WDYT about $first, $second, ...$rest? ( renaming arguments is not BC break, we can do it now )

@nzour
Copy link
Contributor Author

nzour commented Apr 26, 2021

You right, first, second and rest sounds more logic, I see it now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Medium This issue may be useful, and needs some attention. Status: Accepted It's clear what the subject of the issue is about, and what the resolution should be. Status: In Progress This issue is being worked on, and has someone assigned. Type: Enhancement Most issues will probably ask for additions or changes.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants