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

Allow non-string keys #3221

Merged
merged 1 commit into from
Apr 28, 2020
Merged

Conversation

greg0ire
Copy link
Contributor

Although it would be stupid to provide an array with exclusively
non-string keys, it's possible to have array with a bit of both.

See for instance
https://github.com/doctrine/dbal/blob/155d028be084ef69110d09938c57c351a2126e5d/tests/Doctrine/Tests/DBAL/Functional/DataAccessTest.php#L263-L276

@greg0ire
Copy link
Contributor Author

@muglug I would like to write a test, but I don't understand how they are organized. Can you recommend an existing file, or should I create a new one?

@greg0ire greg0ire force-pushed the lenient-array-change-key-case branch from 8223d30 to 333a51b Compare April 24, 2020 19:28
@@ -249,11 +249,12 @@ function uksort(array &$arr, callable $callback): bool
}

/**
* @psalm-template K of array-key
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 maybe I should remove the of array-key part?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You may do that, however it really shouldn't do any harm. After all, array keys could only be array-key. Come to think of it, it may even be useful to catch situations when Psalm wrongly infers key types, like here: https://psalm.dev/r/5c1dae6bd1

@greg0ire greg0ire mentioned this pull request Apr 25, 2020
16 tasks
@greg0ire
Copy link
Contributor Author

greg0ire commented Apr 27, 2020

Can you recommend an existing file

Answering my own question: https://github.com/vimeo/psalm/blob/master/tests/ArrayFunctionCallTest.php looks like a good candidate

@greg0ire greg0ire force-pushed the lenient-array-change-key-case branch from 333a51b to 082c1fa Compare April 27, 2020 19:42
@greg0ire greg0ire marked this pull request as ready for review April 27, 2020 20:05
@greg0ire
Copy link
Contributor Author

The same with a test, please review :)

Although it would be stupid to provide an array with exclusively
non-string keys, it's possible to have an array with a bit of both.

See for instance
https://github.com/doctrine/dbal/blob/155d028be084ef69110d09938c57c351a2126e5d/tests/Doctrine/Tests/DBAL/Functional/DataAccessTest.php#L263-L276
@greg0ire greg0ire force-pushed the lenient-array-change-key-case branch from 082c1fa to 8c1a183 Compare April 27, 2020 20:30
@muglug muglug merged commit 1fb1c21 into vimeo:master Apr 28, 2020
@muglug
Copy link
Collaborator

muglug commented Apr 28, 2020

Thanks!

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.

3 participants