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

added phpstan config with baseline #126

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

connorhu
Copy link
Collaborator

@connorhu connorhu commented Feb 22, 2024

the baseline file contains errors about #119

@connorhu
Copy link
Collaborator Author

connorhu commented Feb 22, 2024

If the test folder were not a mess, I would put the baseline file there.

@thePanz
Copy link
Member

thePanz commented Feb 22, 2024

the baseline is quite big, we should use the php format of the baseline to improve performances in the analysis

hint: just generate the baseline with .php as exension, phpstan will do the rest ;-)

@thePanz
Copy link
Member

thePanz commented Feb 22, 2024

We should use phpstan.dist.neon as IDEs tend to recognize the type by the extension (this still works for phpstan)

@connorhu
Copy link
Collaborator Author

connorhu commented Feb 22, 2024

the baseline is quite big, we should use the php format of the baseline to improve performances in the analysis

hint: just generate the baseline with .php as exension, phpstan will do the rest ;-)

1.3M Feb 22 17:38 phpstan-baseline.neon
1.6M Feb 22 17:40 phpstan-baseline.php

neon is smaller

@connorhu connorhu force-pushed the feature/phpstan branch 3 times, most recently from ac2e5e4 to 41684a2 Compare February 22, 2024 16:44
@thePanz
Copy link
Member

thePanz commented Feb 22, 2024

the baseline is quite big, we should use the php format of the baseline to improve performances in the analysis
hint: just generate the baseline with .php as exension, phpstan will do the rest ;-)

1.3M Feb 22 17:38 phpstan-baseline.neon
1.6M Feb 22 17:40 phpstan-baseline.php

neon is smaller

True, but: https://phpstan.org/user-guide/baseline#php-baseline-format-instead-of-neon

This might help with parsing performance and memory consumption in excessive baseline files that are megabytes in size. PHP code will be interpreted quicker than an equivalent .neon file.

WDYT?

php-version: 8.3
tools: cs2pr

- name: Install dependencies with Composer
Copy link
Member

@thePanz thePanz Feb 22, 2024

Choose a reason for hiding this comment

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

I would suggest to simplify this action and introduce a composer cache like suggested here:

https://github.com/shivammathur/setup-php?tab=readme-ov-file#cache-composer-dependencies

Something like:

- name: Get composer cache directory
  id: composer-cache
  run: echo "dir=$(composer config cache-files-dir)" >> $GITHUB_OUTPUT

- name: Cache dependencies
  uses: actions/cache@v4
  with:
    path: ${{ steps.composer-cache.outputs.dir }}
    key: ${{ runner.os }}-composer-${{ hashFiles('**/composer.json') }}
    restore-keys: ${{ runner.os }}-composer-

- name: Install dependencies
  run: composer install --prefer-dist```

@connorhu
Copy link
Collaborator Author

the baseline is quite big, we should use the php format of the baseline to improve performances in the analysis
hint: just generate the baseline with .php as exension, phpstan will do the rest ;-)

1.3M Feb 22 17:38 phpstan-baseline.neon
1.6M Feb 22 17:40 phpstan-baseline.php

neon is smaller

True, but: https://phpstan.org/user-guide/baseline#php-baseline-format-instead-of-neon

This might help with parsing performance and memory consumption in excessive baseline files that are megabytes in size. PHP code will be interpreted quicker than an equivalent .neon file.

WDYT?

I missed that point of your argue.

@thePanz
Copy link
Member

thePanz commented Feb 23, 2024

I missed that point of your argue.
No problem, i had a performance issue on another project for that reason, thus I would always opt for the php version of the baseline now :)

Thanks for modernizing the project! 👍

@connorhu
Copy link
Collaborator Author

connorhu commented Feb 23, 2024

No problem, i had a performance issue on another project for that reason, thus I would always opt for the php version of the baseline now :)

Yesterday I couldn't find any reference to how to test if we need to support php7 and 8, but today I found Ondrej's suggestion:

phpstan/phpstan#4060 (comment)

Based on that, I need to do a bit of reworking.

@connorhu connorhu marked this pull request as draft February 23, 2024 08:11
@connorhu
Copy link
Collaborator Author

I don't see a huge difference in the baseline files generated with the php7 and php8 version settings. Maybe it's safe to say that we generate with the lowest supported (php7.4) and highest supported (php8.3) versions and always test on them

@connorhu connorhu marked this pull request as ready for review February 23, 2024 19:14
@connorhu
Copy link
Collaborator Author

I think I'm done here. I don't know how much hair will fall out from testing with 2 php versions at the same time, but the basic analyze command runs 83, you can run both separately, CI runs both.

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