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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bye Psalm 馃憢馃徏 #15582

Merged
merged 1 commit into from Dec 4, 2023
Merged

Bye Psalm 馃憢馃徏 #15582

merged 1 commit into from Dec 4, 2023

Conversation

jakubtobiasz
Copy link
Member

Q A
Branch? 1.12
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Related tickets n/a
License MIT

Justification

Hello everyone, last Friday, we once again raised the topic of maintaining two static analysis tools within Sylius. We're wondering about switching to only one (preferably phpstan, with a change to write custom PHPStan rules for Sylius).

Why? From time to time we have to face with issues when one tool is fine with some code, but another starts complaining. We're aware it (probably) increases the code quality, but at the same time, some of us find the ROI at a low level compared to (stupid) work we sometimes have to do.

Core Team Voting result:
6 votes for keeping only PHPStan
1 vote for keeping only Psalm
1 vote for keeping both
2 votes for keeping any but only one

I was wondering whether we should target it against 1.12 or 1.13, but as it's not a hard dependency, and we still maintain 1.12, I decided to pick this one.

@jakubtobiasz jakubtobiasz added the DX Issues and PRs aimed at improving Developer eXperience. label Nov 30, 2023
@jakubtobiasz jakubtobiasz requested a review from a team as a code owner November 30, 2023 17:41
@probot-autolabeler probot-autolabeler bot added API APIs related issues and PRs. Maintenance CI configurations, READMEs, releases, etc. labels Nov 30, 2023
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 30, 2023 •
edited

Bunnyshell Preview Environment deleted

Available commands:

  • /bns:deploy to redeploy the environment

diimpp
diimpp reviewed Nov 30, 2023
View reviewed changes
Copy link
Member

@diimpp diimpp left a comment

Choose a reason for hiding this comment

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

So Long, Psalm, and Thanks for All the Fish 馃槃

src/Sylius/Component/Core/Model/ProductImagesAwareInterface.php
* @return Collection|ImageInterface[]
*
* @psalm-return Collection<array-key, ImageInterface>
* @return Collection<array-key, ImageInterface>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @return Collection<array-key, ImageInterface>
* @return Collection<array-key, ProductImageInterface>

src/Sylius/Component/Core/Model/ProductImagesAwareInterface.php
* @return Collection|ImageInterface[]
*
* @psalm-return Collection<array-key, ImageInterface>
* @return Collection<array-key, ImageInterface>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @return Collection<array-key, ImageInterface>
* @return Collection<array-key, ProductImageInterface>

src/Sylius/Component/Core/Model/ChannelInterface.php
* @return Collection|CountryInterface[]
*
* @psalm-return Collection<array-key, CountryInterface>
* @return Collection<array-key, CountryInterface>
Copy link
Member

@diimpp diimpp Nov 30, 2023 •
edited

Choose a reason for hiding this comment

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

returns so far using multi-line syntax, is it possible to convert them to single line as well?

src/Sylius/Component/Core/Model/ProductVariant.php
/**
* @psalm-suppress InvalidReturnType https://github.com/doctrine/collections/pull/220
* @psalm-suppress InvalidReturnStatement https://github.com/doctrine/collections/pull/220
*/
public function getImagesByType(string $type): Collection
{
/** @var Collection<array-key, ImageInterface> $imagesByType */
Copy link
Member

Choose a reason for hiding this comment

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

This docblock shouldn't be needed anymore.

Copy link
Member

Choose a reason for hiding this comment

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

On line 297, Assert::allIsInstanceOf($imagesByType, ImageInterface::class); does look needed.

src/Sylius/Component/Core/Model/ProductVariant.php
/**
* @psalm-suppress InvalidReturnType https://github.com/doctrine/collections/pull/220
* @psalm-suppress InvalidReturnStatement https://github.com/doctrine/collections/pull/220
*/
public function getImages(): Collection
{
/** @phpstan-ignore-next-line */
Copy link
Member

Choose a reason for hiding this comment

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

If ProductImagesAwareInterface docblocks would be fixed from ImageInterface to ProductImageInterface, then this ignore is probably won't be needed anymore.

src/Sylius/Component/Core/Model/Taxon.php
*
* @psalm-var Collection<array-key, ImageInterface>
*/
/** @var Collection<array-key, ImageInterface> */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/** @var Collection<array-key, ImageInterface> */
/** @var Collection<array-key, TaxonImageInterface> */

Copy link
Member

Choose a reason for hiding this comment

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

Line 36

        /** @var ArrayCollection<array-key, ImageInterface> $this->images */
        $this->images = new ArrayCollection();

Could those docblocks be merged somehow?

Copy link
Member

Choose a reason for hiding this comment

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

--- return $this->images->filter(function (ImageInterface $image) use ($type): bool {
+++ return $this->images->filter(function (TaxonImageInterface $image) use ($type): bool {

Can we change this without breaking BC?

src/Sylius/Component/Core/Model/Product.php Outdated Show resolved Hide resolved
src/Sylius/Component/Product/Model/Product.php
*
* @psalm-var Collection<array-key, ProductAssociationInterface>
*/
/** @var Collection<array-key, ProductAssociationInterface> */
Copy link
Member

Choose a reason for hiding this comment

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

Can those docblocks be merged somehow?

        /** @var ArrayCollection<array-key, AttributeValueInterface> $this->attributes */
        $this->attributes = new ArrayCollection();
        /** @var ArrayCollection<array-key, ProductAssociationInterface> $this->associations */
        $this->associations = new ArrayCollection();
        /** @var ArrayCollection<array-key, ProductVariantInterface> $this->variants */
        $this->variants = new ArrayCollection();
        /** @var ArrayCollection<array-key, ProductOptionInterface> $this->options */
$this->options = new ArrayCollection();

Copy link
Member

Choose a reason for hiding this comment

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

If yes, then it's actual for the rest of entities with collections as well, though it's probably outside of current PR scope.

src/Sylius/Component/Promotion/Model/CatalogPromotion.php
*
* @psalm-var Collection<array-key, CatalogPromotionScopeInterface>
*/
/** @var Collection<array-key, CatalogPromotionScopeInterface> */
protected Collection $scopes;

protected Collection $actions;
Copy link
Member

Choose a reason for hiding this comment

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

Needs array shape as well?

@vvasiloi vvasiloi mentioned this pull request Dec 1, 2023
Closed
NoResponseMate
NoResponseMate approved these changes Dec 4, 2023
View reviewed changes
Copy link
Contributor

@NoResponseMate NoResponseMate left a comment

Choose a reason for hiding this comment

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

@diimpp
We'll go over the leftover missing @var's, probably using some automated tool since it's tedious; for now I'm merging it as is just to get rid of psalm 馃拑

@NoResponseMate NoResponseMate merged commit c227331 into Sylius:1.12 Dec 4, 2023
25 checks passed
@NoResponseMate
Copy link
Contributor

Thank you, @jakubtobiasz!

@jakubtobiasz jakubtobiasz deleted the no-more-psalm branch December 4, 2023 10:44
@Rafikooo Rafikooo mentioned this pull request Jan 18, 2024
Merged
NoResponseMate added a commit to Sylius/PayPalPlugin that referenced this pull request Jan 18, 2024
@NoResponseMate
minor #283 Bye Psalm 馃憢 (Rafikooo)
8f63d74 
This PR was merged into the 1.5 branch.

Discussion
----------

| Q               | A
| --------------- | -----
| Branch?         | master
| Bug fix?        | no
| New feature?    | no
| Related tickets | Sylius/Sylius#15582


Commits
-------
  [Composer] Remove Psalm
  [Psalm] Remove configuration file
  [Psalm] Remove outdated annotations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@diimpp diimpp diimpp left review comments

@NoResponseMate NoResponseMate NoResponseMate approved these changes

Assignees
No one assigned
Labels
API APIs related issues and PRs. DX Issues and PRs aimed at improving Developer eXperience. Maintenance CI configurations, READMEs, releases, etc.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jakubtobiasz @NoResponseMate @diimpp