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

Dropped support for PHP 8.0 and updated Pest to v2 #36

Merged
merged 5 commits into from
Oct 18, 2023
Merged

Dropped support for PHP 8.0 and updated Pest to v2 #36

merged 5 commits into from
Oct 18, 2023

Conversation

AmooAti
Copy link
Contributor

@AmooAti AmooAti commented Oct 17, 2023

Hi,
Hope you're doing well.
In this PR, I changed the minimum PHP version to 8.1 and updated Pest PHP to version 2 (Closes #33).
Since Pest 2 uses PHPUnit v10, we had to update the old phpunit.xml to match the newer version's settings. I used ./vendor/bin/pest --migrate-configuration command to upgrade the file. I also removed some settings like stopOnFailure and processIsolation since they were set to the default value of PHPUnit.

I understand the concept of mono PR, but on the way to updating, I fixed a function's doctype and ran the format script. :D

@roberto-butti
Copy link
Contributor

Hi @AmooAti thank you for this great PR.
I appreciate your effort, high quality, and yes, welcome in the statistics repository as contributor.
The PR, I think is fine, but the static code analysis fails in the Github actions, not because your changes, but simply because updating the phpstan library, now it detects an additional use case:

Run vendor/bin/phpstan analyse src --level 9 --error-format=github --no-progress --no-ansi
Note: Using configuration file /home/runner/work/statistics/statistics/phpstan.neon.
Error: Parameter #1 $value of function intval expects array|bool|float|int|resource|string|null, mixed given.
Error: Parameter #1 $value of function floatval expects array|bool|float|int|resource|string|null, mixed given.
Error: Parameter #1 $value of function floatval expects array|bool|float|int|resource|string|null, mixed given.
Error: Parameter #1 $value of function floatval expects array|bool|float|int|resource|string|null, mixed given.
Error: Parameter #1 $value of function floatval expects array|bool|float|int|resource|string|null, mixed given.
Error: Process completed with exit code 1.

So I can follow one of the two approaches.

  • merge your PR, and then open a specific new issue for resolving the phpstan issue
  • or if you want you can fix also this issue on this PR

As you mention, for the single responsibility of the PR, i would prefer the first option.
@AmooAti what do you think ?

@AmooAti
Copy link
Contributor Author

AmooAti commented Oct 18, 2023

Hi @roberto-butti,
I'm glad this PR gets your approval, and thank you so much for your welcoming message.
Yes, I saw the errors but was wondering about the single responsibility of the PR, so I waited for your review.
Like you, I agree with the first option since this PR blocks the development process of another issue.

@roberto-butti
Copy link
Contributor

Totally agree with you, and thank you for the feedback @AmooAti
It is amazing to build together with people with positive approach, kind, and want to improve.

@AmooAti can I ask you a favor before i merge the PR: could you decrease the level of PHPstan , form level 9 to level 8 in the githubactions?
The file is : .github/workflows/static-code-analysis.yml, and push the commi in this PR (so we can skip error), I will open a new issue specific for fixing the new level 9 of phpstan. Thank you

@AmooAti
Copy link
Contributor Author

AmooAti commented Oct 18, 2023

@roberto-butti
Thank, I'm happy to find this amazing repo and have the chance to work with you on this project. I got fascinated when I ran coverage and then saw the actual code.
I decreased it.

@roberto-butti
Copy link
Contributor

@AmooAti this is great! thank you for your contribution. I'm going to merge it

@roberto-butti roberto-butti merged commit a748bf7 into Hi-Folks:main Oct 18, 2023
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.

Dropping support for PHP8.0 and use Pest 2
2 participants