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

Improve static analysis tests #153

Merged
merged 16 commits into from
Jul 28, 2021
Merged

Improve static analysis tests #153

merged 16 commits into from
Jul 28, 2021

Conversation

drupol
Copy link
Collaborator

@drupol drupol commented Jul 18, 2021

This PR:

  • Has static analysis tests (psalm, phpstan)

This PR depends on #161

@drupol drupol requested a review from AlexandruGG July 18, 2021 17:26
@drupol drupol self-assigned this Jul 18, 2021
@drupol drupol force-pushed the refactor/improve-sa-tests branch from 5257ab0 to 9db7b32 Compare July 18, 2021 17:56
@drupol drupol removed their assignment Jul 18, 2021
@drupol drupol added the enhancement New feature or request label Jul 18, 2021
@drupol drupol force-pushed the refactor/improve-sa-tests branch from 9db7b32 to f0c8f57 Compare July 19, 2021 07:28
@drupol drupol mentioned this pull request Jul 19, 2021
5 tasks
@drupol drupol force-pushed the refactor/improve-sa-tests branch 2 times, most recently from b142df1 to 849e564 Compare July 19, 2021 19:11
@AlexandruGG AlexandruGG mentioned this pull request Jul 20, 2021
5 tasks
@drupol drupol force-pushed the refactor/improve-sa-tests branch from 849e564 to 27228a3 Compare July 20, 2021 10:48
@AlexandruGG
Copy link
Collaborator

👀

@AlexandruGG
Copy link
Collaborator

There has been a phpstan release today which is causing some new errors, I'll see if I can sort them out

@AlexandruGG
Copy link
Collaborator

There has been a phpstan release today which is causing some new errors, I'll see if I can sort them out

Psalm and PHPStan are having some disagreements now on this whole non-empty-string thing 🙄. I bet there's going to be a fix/tweak

@AlexandruGG
Copy link
Collaborator

could you use this commit ff9c4b6 as an example to add some "failure cases" for the other operations? If we are not "stress-testing" the type annotations I'm not confident that they are correct or that the static analysis tests are adding any value

@drupol
Copy link
Collaborator Author

drupol commented Jul 21, 2021

@AlexandruGG I fixed the issue with Reduction, however, regarding the non-empty-string with phpstan I have some issues.
When I fix it, psalm complains... and when I don't phpstan complains!

Do you think we should fix the version of PHPStan until this is fixed upstream?

@AlexandruGG
Copy link
Collaborator

@AlexandruGG I fixed the issue with Reduction, however, regarding the non-empty-string with phpstan I have some issues.
When I fix it, psalm complains... and when I don't phpstan complains!

Do you think we should fix the version of PHPStan until this is fixed upstream?

Yeah I had that issue as well, yeah let's fix the version for a bit and see what happens

@drupol drupol force-pushed the refactor/improve-sa-tests branch 3 times, most recently from c205155 to 217d365 Compare July 23, 2021 06:46
@drupol
Copy link
Collaborator Author

drupol commented Jul 23, 2021

@AlexandruGG I fixed the PHPStan issues, now Psalm complains.

Which version of PHPStan do you advice to use?

@drupol drupol force-pushed the refactor/improve-sa-tests branch 2 times, most recently from e06a298 to 67b0dc6 Compare July 26, 2021 17:01
@drupol drupol changed the base branch from master to fix/phpstan-bug-5372-temporary-workaround July 26, 2021 17:16
@drupol drupol changed the base branch from fix/phpstan-bug-5372-temporary-workaround to master July 26, 2021 17:19
@drupol drupol force-pushed the refactor/improve-sa-tests branch from 5ddf766 to 3065bb8 Compare July 26, 2021 17:55
Comment on lines +26 to +27
* @template V
* @template W
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why this is different from Reduceable.

Same of ScanLeftable and ScanRightable, shouldn't they all be the same?

Copy link
Collaborator Author

@drupol drupol Jul 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, there was an inconsistency, I just fixed it.

However, ScanLeft and ScanRight might return the initial value if the Iterator is empty, this is why they might return V or W but Reduce and Reduction will not return anything, this is why it only returns W.
ScanLeft1 and ScanRight1 uses the first value of the iterator as initial value, so the signature has small differences.

@drupol drupol force-pushed the refactor/improve-sa-tests branch from 3065bb8 to cc428ec Compare July 27, 2021 15:28
@drupol drupol self-assigned this Jul 27, 2021
@drupol drupol marked this pull request as draft July 27, 2021 15:56
@drupol drupol force-pushed the refactor/improve-sa-tests branch from cc428ec to ed5a4cd Compare July 27, 2021 17:13
@drupol drupol marked this pull request as ready for review July 27, 2021 17:18
@drupol drupol removed their assignment Jul 27, 2021
@drupol drupol enabled auto-merge (squash) July 27, 2021 17:37
@drupol drupol merged commit 5325cc4 into master Jul 28, 2021
@drupol drupol deleted the refactor/improve-sa-tests branch July 28, 2021 06:53
@drupol
Copy link
Collaborator Author

drupol commented Jul 28, 2021

Best news of the day! Thanks for the review!

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

Successfully merging this pull request may close these issues.

2 participants