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

Fix error message for call to internal method from root namespace #6328

Merged
merged 1 commit into from
Aug 17, 2021

Conversation

bdsl
Copy link
Contributor

@bdsl bdsl commented Aug 17, 2021

I noticed that the error output linked from https://twitter.com/afilina/status/1425841636282671105 is not grammatical.

@bdsl
Copy link
Contributor Author

bdsl commented Aug 17, 2021

There are probably several other internal code violations that should be detected - e.g. https://psalm.dev/r/8a680925cf but they can be done separately, and I don't know if it's worth fixing them before I or someone else actually encounters a real case of code that should be internal being onerously called.

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/8a680925cf
<?php declare(strict_types = 1);

namespace Vendor1 {
	class Foo
	{
		/**
	 	* @internal
	 	*/
		public int $foo = 42;
	}
}

namespace Vendor2 {
    echo (new \Vendor1\Foo())->foo;
}
Psalm output (using commit cc717f7):

No issues!

@weirdan
Copy link
Collaborator

weirdan commented Aug 17, 2021

I don't know if it's worth fixing them before I or someone else actually encounters a real case of code that should be internal being onerously called.

If you have time and feel the urge then why not? We don't have any policies like 'must affect N users' or anything like that.

@weirdan weirdan merged commit fdd286f into vimeo:master Aug 17, 2021
@weirdan
Copy link
Collaborator

weirdan commented Aug 17, 2021

Thanks!

@bdsl
Copy link
Contributor Author

bdsl commented Aug 17, 2021

I don't know if it's worth fixing them before I or someone else actually encounters a real case of code that should be internal being onerously called.

If you have time and feel the urge then why not? We don't have any policies like 'must affect N users' or anything like that.

Right, I just mean worth the time for me or anyone else who wants to implement the fix. I might do some point in future, not today. I can report the issue if you think that would be useful in case anyone else wants to implement.

@bdsl
Copy link
Contributor Author

bdsl commented Aug 17, 2021

Reported: #6329

@weirdan
Copy link
Collaborator

weirdan commented Aug 17, 2021

I might do some point in future, not today.

Sure, no pressure. We're all volunteers here, there's no obligations of any sort.

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.

None yet

2 participants