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

3.x internal refactor #1075

Merged
merged 10 commits into from
Nov 12, 2018
Merged

3.x internal refactor #1075

merged 10 commits into from
Nov 12, 2018

Conversation

muglug
Copy link
Collaborator

@muglug muglug commented Nov 11, 2018

This changes some terminology:

  • Checker -> Analyzer
  • _checker -> _analyzer

Also migrates a lot of classes whose internals/APIs may change between minor versions into a Psalm\Internal namespace.

Changes the Psalm\Plugin methods so that the args have a more logical order, and so they don't depend on internal classes (this is a breaking change).

@TysonAndre: are there other plugin changes you'd like to make here? Perhaps with regards to #535? I'd be happy to add a --no-class-cache option if it's still relevant

@weirdan can any of the new plugin classes be internal?

@weirdan
Copy link
Collaborator

weirdan commented Nov 11, 2018

can any of the new plugin classes be internal?

All of them could be internal, apart from interfaces in PluginApi namespace.

Changes the Psalm\Plugin methods so that the args have a more logical order, and so they don't depend on internal classes (this is a breaking change).

I have additional proposal of splitting Psalm\Plugin into interfaces: https://github.com/vimeo/psalm/compare/master...weirdan:plugin-interface-segregation?expand=1
and then dropping Psalm\Plugin altogether.

@muglug
Copy link
Collaborator Author

muglug commented Nov 11, 2018

@weirdan

I have additional proposal of splitting Psalm\Plugin into interfaces

That’s awesome - would you be able to PR that against this branch?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 87.582% when pulling 7b3fcc2 on internal-refactor into 052d4f6 on master.

@weirdan
Copy link
Collaborator

weirdan commented Nov 11, 2018

Sure, will do today.

@TysonAndre
Copy link
Contributor

The changes look good, I don't have any objections to the changes here, and I can't think of anything to add/remove off of the top of my head (in terms of backwards incompatible changes), other than changing plugins.

I have additional proposal of splitting Psalm\Plugin into interfaces

A similar approach was taken in Phan with Capabilities. See https://github.com/phan/phan/blob/e7583378eccea20e7c72c00be43b1b2071dcb75a/src/Phan/PluginV2.php#L17-L81

I'd be happy to add a --no-class-cache option if it's still relevant

And it's useful for plugins that want to add types from other sources

It seems like it's easy to add, independent of any breaking changes (i.e. major releases)

See https://github.com/TysonAndre/psalm/blob/94a8262f6e3ab9218ebfd7b82ccce3a56afbe568/src/psalm.php#L411-L433 - It mostly seems like you just need to replace one of the caches with a null cache to do that. I'm not familiar enough with the project to know if this would break any assumptions. Feel free to copy those changes.

@@ -3,7 +3,7 @@

use PhpParser;

class ParserInstanceCacheProvider extends \Psalm\Provider\ParserCacheProvider
class ParserInstanceCacheProvider extends \Psalm\Internal\Provider\ParserCacheProvider
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Would it make more sense to move tests/Provider to tests/Internal/Provider (etc.) so that Psalm\Tests\Internal\Foo contains overrides of Psalm\Internal\Foo (e.g. if both Psalm\Provider and Psalm\Internal\Provider exist later)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, makes sense

@weirdan
Copy link
Collaborator

weirdan commented Nov 11, 2018

That’s awesome - would you be able to PR that against this branch?

@muglug see #1076

@muglug
Copy link
Collaborator Author

muglug commented Nov 12, 2018

@TysonAndre I've added a --no-reflection-cache argument that does the same job. The provider system was designed to be modular (for testing purposes) but it works well here, too.

@muglug muglug merged commit f10714e into master Nov 12, 2018
@muglug muglug deleted the internal-refactor branch November 12, 2018 16:21
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.

4 participants