Skip to content

RenameTestCase to BaseTestCase to prevent import conflicts with PHPUnit's TestCase. #634

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

Conversation

frankdejonge
Copy link
Contributor

I use auto-complete quite a lot, and although PHPStorm tries to be clever around what class name it will actually import, many time I'm importing PHPStan's test case class instead of PHPUnit's version I intended to import. This causes me to manually remove the import and re-import the intended class.

I hope you'll consider merging this PR 🙏

@ondrejmirtes
Copy link
Member

Hi, thanks for this PR. On one hand, I understand why do you want to do this. Renaming it might improve the experience for a lot of users.

On the other hand, I hope you understand the root issue isn't in PHPStan, it's in PhpStorm. PhpStorm is the one behaving badly. It's not PHPStan's fault to have a class named TestCase. There's no real conflict - it's in a different namespace.

But I agree this might be a useful change. I'll merge this for 1.0 in the coming months. I can't merge this now because it's a BC break as pointed out in: https://github.com/phpstan/phpstan-src/pull/634/checks?check_run_id=3382951376

Also this was a useful suggestion: to configure PhpStorm so that it's smarter about autocompletion. https://www.jetbrains.com/help/phpstorm/auto-completing-code.html#ml_completion

@frankdejonge
Copy link
Contributor Author

Hi @ondrejmirtes,

Great to hear your intention to merge for 1.0. I do agree that it would be great if PHPStorm dealt better with the imports. As far as your suggestion for ML-backed suggestions, I've tried it but in general it made the IDE experience worse for me, unfortunately. If at the time of merging there are conflicts, feel free to ping me and I'll get a fresh one that's ready to merge 👍

@BackEndTea
Copy link
Contributor

This could maybe done without a BC break through a class_alias. Would even give people the option to be compatible with both v 0.12 & 1.0.

But that is a bit more work, so im not sure if thats worth the hassle.

@ondrejmirtes
Copy link
Member

If you rebase it now, I'll merge it 😊

@frankdejonge
Copy link
Contributor Author

On it!

@frankdejonge frankdejonge force-pushed the frank/prevent-test-case-import-conflict branch from 4ca2848 to bb98731 Compare September 13, 2021 06:47
@frankdejonge
Copy link
Contributor Author

frankdejonge commented Sep 13, 2021

@ondrejmirtes perhaps we should name it PHPStanTestCase since it's also a public facing interface, will make it easier for people who use autocomplete and not want to scroll through 100 BaseTestCase classes.

What do you think?

@frankdejonge
Copy link
Contributor Author

I've pushed the BaseTestCase => PHPStanTestCase commit, if not desired I can pop it off. Let me know :)

@ondrejmirtes
Copy link
Member

Thank you, rebased and merged: 755bd66

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