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

add phpstan error identifiers #118

Merged
merged 1 commit into from
Jun 13, 2024

Conversation

pilif
Copy link
Contributor

@pilif pilif commented Jun 12, 2024

For easier handling of ignores, PHPStan 1.11 has added error identifiers.

This PR adds them to the various reports generated by the unused-public extension.

As far as I can tell, the built-in testing framework for rules doesn't yet support error identifiers and thus I can't really provide test-cases for these. Correct me if I'm wrong and I'll gladly add test-cases.

@pilif pilif force-pushed the error-identifiers branch from d8b4901 to 91cc926 Compare June 12, 2024 14:27
@TomasVotruba
Copy link
Owner

Thanks for addition! 👍

"Method unused" could suggest its an unused private method. Could you add word "public.*" to make it more clear?

@pilif
Copy link
Contributor Author

pilif commented Jun 13, 2024

I intentionally chose identifiers that were already known to PHPStan, especially given that they are all semantically matching these rules.

I thought that by using existing identifiers, tools which make use of identifiers would work out of the box and would be able to make use of the existing semantics.

Adding a public.* prefix would a) cause these identifiers to be different in format from all existing ones (by having three segments rather than two like all others) and b) cause them to be distinct from the known ones with known semantics

then you look at the list of known identifiers you will see that many rules share identifiers already and none of the identifiers make a difference depending on visibility of where the issue is raised.

I’m totally willing to make the change and I’m absolutely not in the habit of arguing with maintainers (I am very well aware how busy you are), but I would love to get some insight into the why of your request given the considerations above.

@TomasVotruba
Copy link
Owner

Using already existing PHPStan identifiers doesn't seem like a good idea. It's like naming a service with same generic like "manager" string, overriding all existing services with same name. Hence the prefix

@pilif pilif force-pushed the error-identifiers branch from 91cc926 to 964f57e Compare June 13, 2024 06:23
@pilif
Copy link
Contributor Author

pilif commented Jun 13, 2024

force pushed to add the prefix.

@TomasVotruba TomasVotruba enabled auto-merge (squash) June 13, 2024 07:06
@TomasVotruba TomasVotruba merged commit 21592f0 into TomasVotruba:main Jun 13, 2024
8 checks passed
@TomasVotruba
Copy link
Owner

Thank you 🙏

neoighodaro added a commit to neoighodaro/unused-public that referenced this pull request Nov 21, 2024
* TomasVotruba-main: (21 commits)
  Delete .github/FUNDING.yml
  Update FUNDING.yml
  Update composer.json
  Keep the code DRY
  Small improvement
  Added support for @internal or @public
  Reduce memory consumption of collectors (TomasVotruba#131)
  Fix blade regex to discover method call with args (TomasVotruba#128)
  Fix template discovery, to include root file too (TomasVotruba#127)
  remove `composer-dependency-analyser.php` from releases (TomasVotruba#126)
  Bump deps (TomasVotruba#125)
  Detect public properties used via Subclass (TomasVotruba#123)
  Fix ClassConstFetchCollector (TomasVotruba#122)
  Fix union-type handling in PublicStaticPropertyFetchCollector (TomasVotruba#121)
  Fix union-type handling in PublicPropertyFetchCollector (TomasVotruba#120)
  add phpstan error identifiers (TomasVotruba#118)
  Fixed nette/utils indirect dependency (TomasVotruba#116)
  Added test for JsonSerialize (TomasVotruba#112)
  Add RelativeUnusedPublicClassMethodRule (TomasVotruba#111)
  Bump to PHP 8.2 (TomasVotruba#110)
  ...
neoighodaro added a commit to neoighodaro/unused-public that referenced this pull request Nov 21, 2024
* origin/main: (22 commits)
  Allow phpstan 2
  Delete .github/FUNDING.yml
  Update FUNDING.yml
  Update composer.json
  Keep the code DRY
  Small improvement
  Added support for @internal or @public
  Reduce memory consumption of collectors (TomasVotruba#131)
  Fix blade regex to discover method call with args (TomasVotruba#128)
  Fix template discovery, to include root file too (TomasVotruba#127)
  remove `composer-dependency-analyser.php` from releases (TomasVotruba#126)
  Bump deps (TomasVotruba#125)
  Detect public properties used via Subclass (TomasVotruba#123)
  Fix ClassConstFetchCollector (TomasVotruba#122)
  Fix union-type handling in PublicStaticPropertyFetchCollector (TomasVotruba#121)
  Fix union-type handling in PublicPropertyFetchCollector (TomasVotruba#120)
  add phpstan error identifiers (TomasVotruba#118)
  Fixed nette/utils indirect dependency (TomasVotruba#116)
  Added test for JsonSerialize (TomasVotruba#112)
  Add RelativeUnusedPublicClassMethodRule (TomasVotruba#111)
  ...
ngmy pushed a commit to ngmy/unused-public that referenced this pull request Dec 16, 2024
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.

2 participants