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 AssertWpErrorTypeSpecifyingExtension / AssertNotWpErrorTypeSpecifyingExtension #124

Merged
merged 1 commit into from
Nov 10, 2022

Conversation

swissspidy
Copy link
Contributor

Might need a bit of help with the implementation & tests, but I think this should cover the basics.

Fixes #30

@johnbillion
Copy link
Contributor

This might be possible with the new docblock-based assertions instead: https://phpstan.org/blog/phpstan-1-9-0-with-phpdoc-asserts-list-type

@johnbillion
Copy link
Contributor

Although that would mean bumping the minimum PHPStan requirement up to the latest.

@swissspidy
Copy link
Contributor Author

@johnbillion That would mean having to add the docblocks in core, right?

@szepeviktor
Copy link
Owner

szepeviktor commented Nov 10, 2022

bumping the minimum PHPStan requirement up to the latest.

I'm not a maintainer.
I can write code only for myself.

@szepeviktor
Copy link
Owner

...okay let's release v1.2.0 after this PR

@johnbillion
Copy link
Contributor

That would mean having to add the docblocks in core, right?

The https://github.com/php-stubs/wordpress-stubs/ library has a node visitor that allows additional tags to be added to the docblocks for function stubs, but AFAICT the methods in the test suite aren't generated as part of those stubs, so for now I think this type specifying extension makes the most sense actually 👍

@swissspidy swissspidy marked this pull request as ready for review November 10, 2022 14:27
@swissspidy
Copy link
Contributor Author

Just marking as ready for review in the hopes of triggering tests to run... Can't get them to work locally somehow.

@szepeviktor
Copy link
Owner

szepeviktor commented Nov 10, 2022

CI does not start.
Let's see it on master! https://app.travis-ci.com/github/szepeviktor/phpstan-wordpress/builds/257654878

@szepeviktor szepeviktor merged commit 4cc218e into szepeviktor:master Nov 10, 2022
@szepeviktor
Copy link
Owner

szepeviktor commented Nov 10, 2022

@swissspidy There are 2 instances of

--- Expected
+++ Actual
@@ @@
-'WP_Error'
+'int|WP_Error'

@szepeviktor
Copy link
Owner

Importing WP_UnitTestCase_Base may be missing from tests.

@herndlm
Copy link
Contributor

herndlm commented Nov 10, 2022

hey, I still have my phpstorm open from yesterday basically. I can quickly check if I see something :)

@swissspidy
Copy link
Contributor Author

Oh, did not expect this to be merged 😅

There's probably something silly I missed with these extensions. Again, I haven't been able to run the tests locally for some reason, so it was just copy & paste

@herndlm Any help is much appreciated, thank you!

@herndlm
Copy link
Contributor

herndlm commented Nov 10, 2022

Viktor's merge finger was too fast :)

@szepeviktor
Copy link
Owner

Viktor's merge finger was too fast :)

CI was not running in this PR

Please continue in a new PR fixing this one.

@swissspidy swissspidy deleted the fix/30 branch November 10, 2022 15:37
@herndlm
Copy link
Contributor

herndlm commented Nov 10, 2022

I think I figured it out, I'll open a PR explaining it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Treat assertNotWPError() and assertWPError() as type-narrowing functions
4 participants