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

Add an option() type to Psl\Type to coerce to a Psl\Option\Option #418

Open
devnix opened this issue Jul 19, 2023 · 3 comments
Open

Add an option() type to Psl\Type to coerce to a Psl\Option\Option #418

devnix opened this issue Jul 19, 2023 · 3 comments
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: Available No one has claimed responsibility for resolving this issue. Type: Enhancement Most issues will probably ask for additions or changes.

Comments

@devnix
Copy link
Contributor

devnix commented Jul 19, 2023

I'll leave here an example given by @azjezz:

use Psl\Str;
use Psl\Type;
use Psl\Option;

/**
 * @var Type\TypeInterface<Option\OptionInterface<string>>
 */
$type = Type\option(Type\string());

$str = 'abc';
$arr = [];

// = 3
$a = $type->coerce($str)->map(Str\length(...))->unwrapOr(0);
// = 0
$b = $type->coerce($arr)->map(Str\length(...))->unwrapOr(0);

$val = Option\none();
$type->matches($val); // true? `matches` has `@psalm-assert-if-true T $value` so now psalm will think it's `Option<string>`, not an issue

$val = Option\some(123);
$type->matches($val); // false? this would require us to unwrap it if it's a some and check the value

I think I could work on this one

@devnix devnix added the Type: Enhancement Most issues will probably ask for additions or changes. label Jul 19, 2023
@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. Status: Accepted It's clear what the subject of the issue is about, and what the resolution should be. labels Jul 19, 2023
@devnix
Copy link
Contributor Author

devnix commented Aug 4, 2023

I'll be sure to leave here an idea. As we probably want to have fine control of what is some and what is none and each view will likely differ, why not require a callable to decide it?

$myType = Type\option(
    Type\string(),
    static fn (string $value) => '' !== $value
);

$myType->coerce('Hello world')->isSome(); // true
$myType->coerce('')->isSome(); // false

WDYT @azjezz @veewee? I will have a couple of spare weeks so I can start working on this idea if you like it

@veewee
Copy link
Collaborator

veewee commented Aug 4, 2023

Hello @devnix,

I was thinking : wouldn't is make more sense to let the underlying type determine wether it is some or none?
For example:

$type = option(string());
$type->coerce('some'); // Some('some');
$type->coerce(''); // Some('');

// But

$type = option(non_empty_string());
$type->coerce('some'); // Some('some');
$type->coerce(''); // None();

The logic would be as followed:

  • coerce($data)
    • If inner type coerces, then it is some($data)
    • If inner type throws exception, then it is none()
  • assert($data)
    • Validates if $data is of type Option
    • In case of some, validates if inner type asserts on unwrapped option value
  • matches($data)
    • Validates if $data is of type Option
    • In case of some, validates if inner type matches on unwrapped option value

I have not validated every possible type to make sure this logic is 100%, but think it should do the job just fine.

This way, you could use the base types to determine it is a some|none, and a converted() type (instead of a predicate) for very specific scenarios.

WDYT?

@devnix
Copy link
Contributor Author

devnix commented Aug 19, 2023

Hi @veewee, sorry for the late response!

It is true that converted() would be a better candidate for the case I presented.

However, the coerce() behavior looks too forgiving for me. I was surprised by the first time I used shape() when I saw it failed even when the shape was correct, and then I saw that the given shape I was coercing had more keys than my documented shape, and then I learned about the allow_unknown_fields parameter.

I think that it's easier to offer stricter behaviors by default with opt-ins for a more lenient logic. For example:

$type = option(string());
$type->coerce('some'); // Some('some');
$type->coerce(''); // Some('');

$type = option(non_empty_string());
$type->coerce('some'); // Some('some');
$type->coerce(null); // None();
$type->coerce(''); // CoercionException

$type = option(
    non_empty_string(),
    none_if_not_coercible: true
);
$type->coerce('some'); // Some('some');
$type->coerce(''); // None();

However, I also doubt about performing a logic like from_nullable() with code like this:

$type = option(nullable(string()));
$type->coerce('some'); // Some('some');
$type->coerce(null); // Some(null) or None()?

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: Available No one has claimed responsibility for resolving this issue. Type: Enhancement Most issues will probably ask for additions or changes.
Projects
None yet
Development

No branches or pull requests

3 participants