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

consistency in AssertionFinder #7622

Merged
merged 4 commits into from Feb 9, 2022
Merged

consistency in AssertionFinder #7622

merged 4 commits into from Feb 9, 2022

Conversation

orklah
Copy link
Collaborator

@orklah orklah commented Feb 9, 2022

This doesn't do much, it only fixes a small phpdoc error and rearrange a few methods and params to have a good consistency between scrapeEqualityAssertions and scrapeInequalityAssertions

There's a few things that are not directly order changes:

  • The call to getTrueEqualityAssertions didn't pass the inside_conditional value contrary to getTrueInequalityAssertions, getFalseEqualityAssertions and getFalseInequalityAssertions so I added it
  • The count_equality_position check had been put after an early return when we were not dealing with a StatementsAnalyzer. I did that on an earlier PR because I needed the node provider for retrieving types. However, I only needed it for handling paradoxicalAssertions, so I put that back above, like the count_inequality_position check
  • I added the check for paradoxicalAssertions in the count_inequality_position check. (it will possibly find some weird conditions in user code)

Nothing big but it will be cleaner for future changes

@orklah orklah added the release:internal The PR will be included in 'Internal changes' section of the release notes label Feb 9, 2022
@orklah
Copy link
Collaborator Author

orklah commented Feb 9, 2022

Cool, I was able to find a bug too. We were transforming array-key into string as soon as we were seeing an Int assertion, but assertion could have been int ranges or literal int

@orklah orklah merged commit 9984397 into vimeo:master Feb 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:internal The PR will be included in 'Internal changes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant