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

Convertion of partials to full imports #7155

Merged
merged 18 commits into from Dec 14, 2021

Conversation

rarila
Copy link
Contributor

@rarila rarila commented Dec 14, 2021

I finally have converted "all" partial to full imports.

I was looking at all cases but I don’t think readability suffers at any case, as mostly they are unambiguous due prefixing or postfixing or just by naming or context.

One exception is the PhpParser namespace which will need an extra issue for discussion and so is left out here.

@rarila
Copy link
Contributor Author

rarila commented Dec 14, 2021

I guess the error is related to #7153?!

@weirdan weirdan added the release:internal The PR will be included in 'Internal changes' section of the release notes label Dec 14, 2021
@weirdan weirdan self-requested a review December 14, 2021 03:38
@@ -1124,7 +1136,7 @@ public function getSymbolLocation(string $file_path, string $symbol): ?CodeLocat

$file_contents = $this->getFileContents($file_path);

return new CodeLocation\Raw(
return new Raw(
Copy link
Collaborator

@weirdan weirdan Dec 14, 2021

Choose a reason for hiding this comment

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

This is an example of where readability does suffer. Could be renamed to RawLocation though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, Raw is quite inexpressively.
The good thing is, it is used only 4 times. Anyway, DocblockTypeLocation and ParseErrorLocation are postfixed, so this should really be RawLocation.

But I think this should go into an extra PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But I think this should go into an extra PR.

Of course.

@weirdan
Copy link
Collaborator

weirdan commented Dec 14, 2021

I'm halfway through the review, and this is actually not too bad.

Copy link
Collaborator

@weirdan weirdan left a comment

Choose a reason for hiding this comment

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

Note to self: review ParseTree, UnresolvedConstant and CodeLocation class naming.

@rarila
Copy link
Contributor Author

rarila commented Dec 14, 2021

Note to self: review ParseTree, UnresolvedConstant and CodeLocation class naming.

Those two should probably also be added to that list:

Psalm\Internal\LanguageServer\Client\TextDocument // => ClientTextDocument ?!
Psalm\Internal\LanguageServer\Server\TextDocument // => ServerTextDocument ?!

@orklah orklah merged commit 5ddf5df into vimeo:master Dec 14, 2021
@orklah
Copy link
Collaborator

orklah commented Dec 14, 2021

Thanks!

@rarila rarila deleted the partials-conversion-2 branch December 15, 2021 03:02
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

3 participants