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.

Sign up for GitHub

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

Jump to bottom

Add positive-int type assertion via Assert::positiveInt() or similar #213

Closed
Ocramius opened this issue Aug 28, 2020 · 6 comments · Fixed by #214
Closed

Add positive-int type assertion via Assert::positiveInt() or similar #213

Ocramius opened this issue Aug 28, 2020 · 6 comments · Fixed by #214

Comments

@Ocramius
Copy link
Contributor

Psalm now has the positive-int type, so we can add @psalm-assert positive-int $value on a public static function positiveInt(int $value): void.

Note: while I'd love to just have Assert::positive(), it is not really possible to have a type-static assertion that operates on a generic input value of type int|float, therefore we need Assert::positiveInt() as a more specific assertion.
@zerkms
Copy link
Contributor

zerkms commented Aug 28, 2020 •
edited
Loading

If it's to be added - I'd suggest to have mixed $value instead (as an argument type).

@Ocramius
Copy link
Contributor Author

@zerkms wouldn't it be redundant with cases where int is given? I can most certainly try it out though.

@zerkms
Copy link
Contributor

zerkms commented Aug 28, 2020

@Ocramius if you already have an int - it won't be a problem, but if you have mixed it would save from doing one extra Assert::integer().

I already mentioned it couple times in other issues: having too strict assertion input types makes it annoying to assert multiple times in code.

@zerkms
Copy link
Contributor

zerkms commented Aug 28, 2020

@Ocramius oh, I have a better point:

If one relies on type system, then public static function integer($value, $message = '') should not even exist :-P

@Ocramius
Copy link
Contributor Author

@zerkms you know that's not the case when receiving input data from the outside :-)

If we were to write an HTTP request parser that does string -> Maybe int, sure, but in PHP's SAPI system that doesn't work :P

Ocramius added a commit to Ocramius/assert-1 that referenced this issue Aug 28, 2020
@Ocramius
webmozarts#213 implemented Assert::positiveInteger() to have psalm …
57e6622 
…`positive-int` assertions
@Ocramius Ocramius mentioned this issue Aug 28, 2020
Merged
5 tasks
@Ocramius
Copy link
Contributor Author

Implemented in #214

boesing added a commit to boesing/assert that referenced this issue Aug 28, 2020
@boesing
Add keysInteger
ef1a982 
Fixes webmozarts#213

Signed-off-by: Maximilian Bösing <[email protected]>
@boesing boesing mentioned this issue Aug 28, 2020
Closed
boesing added a commit to boesing/assert that referenced this issue Aug 28, 2020
@boesing
Add keysInteger
d7bffa2 
Fixes webmozarts#213

Signed-off-by: Maximilian Bösing <[email protected]>
Ocramius added a commit to Ocramius/assert-1 that referenced this issue Aug 31, 2020
@Ocramius
webmozarts#213 use static::valueToString() instead of `static::type…
d64b56c 
…ToString()` for `Assert::positiveInteger()` failures

Ref: webmozarts#214 (comment)
BackEndTea added a commit to Ocramius/assert-1 that referenced this issue Dec 2, 2020
@BackEndTea
Merge branch 'master' into feature/webmozarts#213-assert-positive-int
df40cef
Ocramius added a commit to Ocramius/assert-1 that referenced this issue Jan 19, 2021
@Ocramius
Implemented webmozarts#213: Assert::positiveInteger() to have psalm…
e6030ef 
… `positive-int` assertions
Ocramius added a commit to Ocramius/assert-1 that referenced this issue Jan 19, 2021
@Ocramius
webmozarts#213 use static::valueToString() instead of `static::type…
7c72c3f 
…ToString()` for `Assert::positiveInteger()` failures

Ref: webmozarts#214 (comment)
Ocramius added a commit to Ocramius/assert-1 that referenced this issue Mar 2, 2021
@Ocramius
Implemented webmozarts#213: Assert::positiveInteger() to have psalm…
ee14f24 
… `positive-int` assertions
Ocramius added a commit to Ocramius/assert-1 that referenced this issue Mar 2, 2021
@Ocramius
webmozarts#213 use static::valueToString() instead of `static::type…
46c8f64 
…ToString()` for `Assert::positiveInteger()` failures

Ref: webmozarts#214 (comment)
Nyholm pushed a commit that referenced this issue Mar 7, 2021
@Ocramius
#213 implemented Assert::positiveInteger() to have psalm `positive-…
6747ed2 
…int` assertions (#214)

* Implemented #213: `Assert::positiveInteger()` to have psalm `positive-int` assertions

* #213 use `static::valueToString()` instead of `static::typeToString()` for `Assert::positiveInteger()` failures

Ref: #214 (comment)

* Added `Assert::naturalNumber` to verify that a number is `positive-int|0`

* Upgraded to latest `vimeo/psalm`

* As suggested by @muglug, dropping `@mixin` and importing a trait as `Assert` type inference mechanism

Ref: https://symfony-devs.slack.com/archives/C8SFXTD2M/p1611064766060600

Quoting:

```
ocramius  2:59 PM
can anyone help me tackle down vimeo/psalm#4381 ?
Mostly trying to figure out whether the assertions are being read differently for @mixin and concrete instances 😐

muglug  3:18 PM
mixins are a massive hack, both whenever they're used but also in Psalm
anything that relies on magic methods to work is going to be more flaky, so I'd encourage you to think of a non-mixin approach if at all possible

ocramius  3:19 PM
yeah, I'm thinking of doing that indeed, just unsure about approach yet 😐
can't they somehow be unified with traits though?
they're the same thing, no?

muglug  3:22 PM
if they were, you'd just use a trait!

ocramius  3:22 PM
ack, gonna actually try that approach for webmozart/assert then 🙂

muglug  3:24 PM
with mixins the method that's actually being called is either __call or __callStatic, whereas with traits it's still that exact method

ocramius  3:24 PM
yes, I was just wondering if it could import the method as if it was a trait (at type level)
that would squash out *a lot* of code, but it's also true that a mixin does not have a trait definition
I think it makes sense to use the language feature for this 🙂
```

The `@mixin` support in `vimeo/psalm` is a massive hack, which also requires other tooling to increase complexity
in the static analysis parsing capabilities. In order to reduce that complexity, we instead rely on `Assert`
importing `Mixin` as a trait, which is much simpler and much more stable long-term.

While this indeed leads to `Mixin` being changed from an `interface` to a `trait`, that is not really a BC
break, as `Mixin` was explicitly documented to be used only as a type system aid, and not as an usable
symbol.

* Removed `Assert::naturalNumber()`, since we already have `Assert::natural()` doing the same thing

Note: we refined the assertion on `Assert::natural()` to correctly restrict to `positive-int|0`,
but we had to remove some minor test on `Assert::allNaturalNumber()` due to `@psalm-assert iterable<positive-int|0>`
not yet working in upstream.

See vimeo/psalm#5052

* Adjusted mixin generator to comply with current coding standards

* Upgraded `vimeo/psalm` to `4.6.1` to get a green build (note: `4.6.2` broken as per vimeo/psalm#5310)

While there is no guarantee that `vimeo/psalm:4.6.3` will fix the issue, we know for sure that
`vimeo/psalm:4.6.2` is broken, as per vimeo/psalm#5310, and we cannot
therefore rely on its static analysis there until upstream issue is fixed.

Meanwhile, this allows for installation of `webmozart/assert` with `vimeo/psalm:4.6.1` and `vimeo/psalm:>4.6.2`,
hoping for a brighter future.
This was referenced Mar 10, 2021
Merged
Closed
Merged
Merged
Merged
Merged
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@Ocramius @zerkms