#213 implemented `Assert::positiveInteger()` to have psalm `positive-int` assertions #214

Ocramius · 2020-08-28T11:27:49Z

Fixes #213

Needs:

Fix for #5049 removing true or false from a Union should turn bool into respectively false or true vimeo/psalm#5050 (and update vimeo/psalm conflict)
~~Regression: @psalm-assert empty not recognized on @mixin imports vimeo/psalm#5051~~ worked around by removing @mixin
~~@psalm-assert iterable<T1|T2> isn't being picked up, and leads to failed type inference vimeo/psalm#5052~~ (skipped - suppressed for now, IMO not a blocker)
~~Regression: in 4.6.2, @psalm-assert iterable<T> does not refine ArrayIterator value vimeo/psalm#5310~~ (not fixed - reported regression in upstream, then locked dependency here)
upgrade vimeo/psalm, update vimeo/psalm conflict constraint

Fixes #213
Fixes #214
Fixes #97
Fixes #229

Ocramius · 2020-08-28T11:34:46Z

No, style-ci, that stuff is not valid/to be fixed :P

src/Assert.php

BackEndTea

Got one minor comment.

I can see the use case for this assertion, especially with prices etc, where 0 would also not be allowed.

src/Assert.php

…ToString()` for `Assert::positiveInteger()` failures Ref: webmozarts#214 (comment)

Ocramius · 2020-08-31T09:38:34Z

CI failure unrelated

Ocramius · 2020-09-29T07:35:36Z

@BackEndTea can we ship this?

Ocramius · 2020-11-06T14:46:18Z

@BackEndTea sorry to page again - can I somehow help here? Can I perhaps help by introducing release automation?

Ocramius · 2021-01-19T12:43:29Z

@BackEndTea sorry, didn't see that you pushed to my fork - currently rebasing to get this merged

…ToString()` for `Assert::positiveInteger()` failures Ref: webmozarts#214 (comment)

Ocramius · 2021-01-19T15:21:22Z

@zerkms you probably want to check this one. As discussed with @muglug, the entire @mixin thing seemed like a good idea, but is really hacks upon hacks upon hacks.

I therefore changed your code generator to generate a trait instead (since the package is now using ^7.2 || ^8.0)

Ocramius · 2021-01-19T15:46:22Z

Note to maintainers: please do not squash this!

Nyholm

Im super impressed. Great work.

Thank you for the rebase.

I see that psalm complains. The error seam valid. Is it related to your PR?

tests/AssertTest.php

Ocramius · 2021-01-19T18:43:56Z

I see that psalm complains. The error seam valid. Is it related to your PR?

That would be the first TODO that is left above - I'm waiting for a merge (if applicable) and release of vimeo/psalm#5050.

zerkms · 2021-01-19T20:09:23Z

@zerkms you probably want to check this one. As discussed with @muglug, the entire @mixin thing seemed like a good idea, but is really hacks upon hacks upon hacks.

I therefore changed your code generator to generate a trait instead (since the package is now using ^7.2 || ^8.0)

@Ocramius I initially was pro-trait/base-class, so it definitely looks good to me :-)

… `positive-int` assertions

…ToString()` for `Assert::positiveInteger()` failures Ref: webmozarts#214 (comment)

…t|0`

@mixin

…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.

…ural()` 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

… 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.

Ocramius · 2021-03-02T15:28:47Z

@BackEndTea @Nyholm I've now rebased and everything looks OK.

Only failure seems to be with scrutinizer-ci, which doesn't seem to accept coverage anymore:


#!/usr/bin/env php
Uploading code coverage for repository "g/webmozarts/assert" and revision "8f1ec4c49eea2fec54bb8ec563bd8d73ac0a3eb9"... Failed
{
    "message": "Not found",
    "description": "The requested page was not found, or you have no access to view it."
}

Meanwhile, I reported vimeo/psalm#5310 and excluded vimeo/psalm:4.6.2 from compatibility, hoping it will somehow be fixed (although optimistically, locking locally in ci/composer.lock is an acceptable solution for us for now).

If you have time, let's roll with this, as it fixes a gazillion issues, and could really need a new release \o/

Nyholm

A gazillion issues fixed in one PR? This is great.

I've reviewed the code and I'm happy with merging.

Thank you Marco

Ocramius · 2021-03-02T15:33:40Z

@Nyholm since the last review, the only change is 6d7d868 BTW (the rest is rebase noise)

Ocramius · 2021-03-03T15:08:40Z

@Nyholm was the merge held off then? 🤔

Nyholm · 2021-03-03T20:26:49Z

I wanted to wait a day or two for @BackEndTea to have a chance to review it again.

Nyholm · 2021-03-07T09:11:30Z

Thank you

Ocramius · 2021-03-08T13:41:53Z

@Nyholm @BackEndTea thank you! Eager to try it out, lemme know if there's anything I can help with to get it released.

If releases are currently onerous, I can help setting up automation, like I did for https://github.com/webmozarts/glob

Nyholm · 2021-03-08T14:29:59Z

I've seen this https://github.com/webmozarts/glob/blob/4.4.x/.github/workflows/release-on-milestone-closed.yml

My mind has not matured for this yet. =)

I'll get there eventually.

Nyholm · 2021-03-09T10:43:13Z

Note to maintainers: please do not squash this!

Oh no!

All of yesterday, I had this feeling that I've read this note somewhere but I could not remember where. I remembered.

Sorry, I did squash the commits.

Unrelated, Im preparing the release now.

Ocramius · 2021-03-09T11:10:11Z

Heh, I mostly did say that because, if you look at the patch history, the commits are made individually, each with a decent commit message, useful to pinpoint regressions :(

Still happy about a release though, but as a good habit, squashing is the exception, not the norm :P

Nyholm · 2021-03-09T11:14:31Z

If one is making commits like you did in this PR, I can be okey with not squashing. But that is rarely what I see, and I've never done it myself. =)

I like the clean look of the history when one squash the commits.

Anyhow, I appreciate all the work you've put into making this PR.

Ocramius mentioned this pull request Aug 28, 2020

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

Closed

staabm reviewed Aug 28, 2020

View reviewed changes

src/Assert.php Show resolved Hide resolved

BackEndTea requested changes Aug 28, 2020

View reviewed changes

src/Assert.php Outdated Show resolved Hide resolved

Ocramius added a commit to Ocramius/assert-1 that referenced this pull request Aug 31, 2020

webmozarts#213 use static::valueToString() instead of `static::type…

d64b56c

…ToString()` for `Assert::positiveInteger()` failures Ref: webmozarts#214 (comment)

Ocramius requested a review from BackEndTea August 31, 2020 09:38

Ocramius added a commit to Ocramius/assert-1 that referenced this pull request Jan 19, 2021

webmozarts#213 use static::valueToString() instead of `static::type…

7c72c3f

…ToString()` for `Assert::positiveInteger()` failures Ref: webmozarts#214 (comment)

Ocramius force-pushed the feature/#213-assert-positive-int branch from 727bbbc to 864876c Compare January 19, 2021 12:44

Ocramius force-pushed the feature/#213-assert-positive-int branch 5 times, most recently from 349e997 to da39c25 Compare January 19, 2021 15:35

Nyholm reviewed Jan 19, 2021

View reviewed changes

tests/AssertTest.php Show resolved Hide resolved

Nyholm added the enhancement label Jan 19, 2021

Ocramius added 7 commits March 2, 2021 16:09

Implemented webmozarts#213: Assert::positiveInteger() to have psalm…

ee14f24

… `positive-int` assertions

webmozarts#213 use static::valueToString() instead of `static::type…

46c8f64

…ToString()` for `Assert::positiveInteger()` failures Ref: webmozarts#214 (comment)

Added Assert::naturalNumber to verify that a number is `positive-in…

103f97e

…t|0`

Upgraded to latest vimeo/psalm

e998b90

Adjusted mixin generator to comply with current coding standards

16b82da

Ocramius force-pushed the feature/#213-assert-positive-int branch from da39c25 to 6d7d868 Compare March 2, 2021 15:23

Nyholm approved these changes Mar 2, 2021

View reviewed changes

BackEndTea approved these changes Mar 5, 2021

View reviewed changes

Nyholm merged commit 6747ed2 into webmozarts:master Mar 7, 2021

Ocramius deleted the feature/#213-assert-positive-int branch March 8, 2021 13:40

This was referenced May 6, 2021

Is findUnusedCode default to true now? vimeo/psalm#5714

Closed

@psalm-pure + @psalm-assert declared on @mixin referenced interfaces are considered UnusedMethodCall when used vimeo/psalm#4381

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#213 implemented `Assert::positiveInteger()` to have psalm `positive-int` assertions #214

#213 implemented `Assert::positiveInteger()` to have psalm `positive-int` assertions #214

Ocramius commented Aug 28, 2020 •

edited

Loading

Ocramius commented Aug 28, 2020

BackEndTea left a comment

Ocramius commented Aug 31, 2020

Ocramius commented Sep 29, 2020

Ocramius commented Nov 6, 2020

Ocramius commented Jan 19, 2021

Ocramius commented Jan 19, 2021 •

edited

Loading

Ocramius commented Jan 19, 2021

Nyholm left a comment

Ocramius commented Jan 19, 2021

zerkms commented Jan 19, 2021

Ocramius commented Mar 2, 2021

Nyholm left a comment

Ocramius commented Mar 2, 2021

Ocramius commented Mar 3, 2021

Nyholm commented Mar 3, 2021

Nyholm commented Mar 7, 2021

Ocramius commented Mar 8, 2021

Nyholm commented Mar 8, 2021

Nyholm commented Mar 9, 2021

Ocramius commented Mar 9, 2021

Nyholm commented Mar 9, 2021

#213 implemented Assert::positiveInteger() to have psalm positive-int assertions #214

#213 implemented Assert::positiveInteger() to have psalm positive-int assertions #214

Conversation

Ocramius commented Aug 28, 2020 • edited Loading

Ocramius commented Aug 28, 2020

BackEndTea left a comment

Choose a reason for hiding this comment

Ocramius commented Aug 31, 2020

Ocramius commented Sep 29, 2020

Ocramius commented Nov 6, 2020

Ocramius commented Jan 19, 2021

Ocramius commented Jan 19, 2021 • edited Loading

Ocramius commented Jan 19, 2021

Nyholm left a comment

Choose a reason for hiding this comment

Ocramius commented Jan 19, 2021

zerkms commented Jan 19, 2021

Ocramius commented Mar 2, 2021

Nyholm left a comment

Choose a reason for hiding this comment

Ocramius commented Mar 2, 2021

Ocramius commented Mar 3, 2021

Nyholm commented Mar 3, 2021

Nyholm commented Mar 7, 2021

Ocramius commented Mar 8, 2021

Nyholm commented Mar 8, 2021

Nyholm commented Mar 9, 2021

Ocramius commented Mar 9, 2021

Nyholm commented Mar 9, 2021

#213 implemented `Assert::positiveInteger()` to have psalm `positive-int` assertions #214

#213 implemented `Assert::positiveInteger()` to have psalm `positive-int` assertions #214

Ocramius commented Aug 28, 2020 •

edited

Loading

Ocramius commented Jan 19, 2021 •

edited

Loading