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

webmozarts / assert

Assertions to validate method input/output with nice error messages.

MIT License

7.56k stars 145 forks source link

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

Closed Ocramius closed 3 years ago

Ocramius commented 4 years ago

Fixes #213

Needs:

[x] https://github.com/vimeo/psalm/pull/5050 (and update vimeo/psalm conflict)
[x] ~~https://github.com/vimeo/psalm/issues/5051~~ worked around by removing @mixin
[x] ~~https://github.com/vimeo/psalm/issues/5052~~ (skipped - suppressed for now, IMO not a blocker)
[x] ~~https://github.com/vimeo/psalm/issues/5310~~ (not fixed - reported regression in upstream, then locked dependency here)
[x] upgrade vimeo/psalm, update vimeo/psalm conflict constraint

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

Ocramius commented 4 years ago

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

Ocramius commented 4 years ago

CI failure unrelated

Ocramius commented 4 years ago

@BackEndTea can we ship this?

Ocramius commented 4 years ago

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

Ocramius commented 3 years ago

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

Ocramius commented 3 years ago

@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 commented 3 years ago

Note to maintainers: please do not squash this!

Ocramius commented 3 years ago

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 commented 3 years ago

@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 :-)

Ocramius commented 3 years ago

@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 https://github.com/vimeo/psalm/issues/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/

Ocramius commented 3 years ago

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

Ocramius commented 3 years ago

@Nyholm was the merge held off then? :thinking:

Nyholm commented 3 years ago

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

Nyholm commented 3 years ago

Thank you

Ocramius commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.