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 union-type handling in PublicPropertyFetchCollector #120

Merged
merged 2 commits into from
Jul 15, 2024

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Jul 15, 2024

closes #119

@staabm staabm changed the title Fix union-type handling in public-properties Fix union-type handling in src/Collectors/PublicPropertyFetchCollector.php Jul 15, 2024
@staabm staabm changed the title Fix union-type handling in src/Collectors/PublicPropertyFetchCollector.php Fix union-type handling in PublicPropertyFetchCollector Jul 15, 2024
Comment on lines +113 to +114
yield [[__DIR__ . '/Fixture/UsedInUnionA.php', __DIR__ . '/Fixture/UsedInUnionB.php', __DIR__ . '/Source/UsedInUnion.php'], []];
yield [[__DIR__ . '/Fixture/UsedInUnionA.php', __DIR__ . '/Fixture/UsedInUnionB.php', __DIR__ . '/Source/UsedInUnionPhpdoc.php'], []];
Copy link
Contributor Author

@staabm staabm Jul 15, 2024

Choose a reason for hiding this comment

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

btw: IMO as someone used to the 1-file test-cases from the phpstan-src repo, this separation of classes over several files (fixture + source) makes creating unit tests pretty hard and unnecessary complex

Copy link
Owner

Choose a reason for hiding this comment

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

What do you suggest?

Copy link
Contributor Author

@staabm staabm Jul 15, 2024

Choose a reason for hiding this comment

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

I would put all classes for a single test-case into a single file (with a unique namespace) and use composer class-map autoloading for tests instead of psr-4.

see https://github.com/phpstan/phpstan-src/blob/89636d6983c73687a9c6d50deac0b01158291f59/composer.json#L133-L136

but not in this PR of course.. it fixes a problem unrelated to this file-system design thing ;)

Copy link
Owner

Choose a reason for hiding this comment

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

I see. For the /Source it made mess as people used same-named classes: )

Copy link
Contributor Author

@staabm staabm Jul 15, 2024

Choose a reason for hiding this comment

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

for the PR we have here, that would mean the test-case file would look like:

namespace Bug119;

class UsedInUnionA {
    public float $amount;
}

class UsedInUnionB {
    public float $amount;
}

function doFooBar(UsedInUnionA|UsedInUnionB $aOrB): void {
    echo $aOrB->amount;
}

  • no separation in fixture/source
  • no complicated file path mapping in the *RuleTest.php
  • easier handling, because a test-case can be deleted with a single file (no dependencies outside the test-file)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same-named classes: )

therefore a dedicated namespace for each test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in phpstan-src there is a CI check to make sure class-names are unique - shipmonk/name-collision-detector

Copy link
Owner

Choose a reason for hiding this comment

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

therefore a dedicated namespace for each test.

That's the place people failed :) we had dozens such cases in Rector, often easy to miss in IDE.

@TomasVotruba TomasVotruba merged commit da62379 into TomasVotruba:main Jul 15, 2024
8 checks passed
@TomasVotruba
Copy link
Owner

Looks good, thanks 👍

@staabm staabm deleted the props branch July 15, 2024 11:43
@staabm
Copy link
Contributor Author

staabm commented Jul 15, 2024

Looks good, thanks

thanks. I prepare another PR with the same fix for static properties

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 19, 2024
…#120)

* Fix union-type handling in public-properties

* simplify
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.

union type support
2 participants