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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

PHPStan enhancements #612

Merged
merged 3 commits into from Dec 4, 2023
Merged

Conversation

Wirone
Copy link
Contributor

@Wirone Wirone commented Dec 1, 2023

Details in commits' messages.

Strict rules and bleeding edge were enabled without any fixes because I need your feedback if you agree to enable them. If yes, I suggest merging this as-is and I can try to work on it in subsequent PR and shrink the baseline a little 馃檪. Removed from this PR.

It allows per-developer customisation (e.g. editor URL). This approach is officially suggested: https://phpstan.org/config-reference#config-file
Last commit in the repo is 4 months old, many new versions of PHPStan were released since then 馃槈.
@stof
Copy link
Member

stof commented Dec 1, 2023

is the PHP baseline as strict conversion of the existing neon baseline or are there new errors as well ? The conversion makes it impossible to determine that in the review.

Also, does phpstan automatically detects the PHP format when using --regenerate-baseline now ? Last time I checked, using a PHP baseline required passing the filename explicitly which makes it harder to contributors. As our baseline is quite small, I'm not sure we need to convert it to PHP format which is not the default.

Also, regarding the phpstan.neon file, I'm not sure it makes sense to move to .dist and ignoring phpstan.neon. I don't see a use case for using a custom phpstan configuration locally (and if you have such temporary use case, you can still use a custom need)

@Wirone
Copy link
Contributor Author

Wirone commented Dec 1, 2023

  1. As stated in the commits' messages, new errors were dumped to baseline because of strict types package and bleeding edge (each commit shows what new errors occurred).
  2. Unfortunately, it's required to pass baseline's file name explicitly. I suggest adding phpstan and phpstan:baseline Composer scripts that would make it transparent for developers. You can see it here, it works great because you can change the underlying command and usage is still the same.
  3. This is approach officially suggested by PHPStan's documentation (it was pointed in the commit's message). Since phpstan.dist.neon is read by default with lower priority, for people that don't make custom config it's 100% transparent. For people who want customise config phpstan.neon is also read by default, but with higher priority. So in the end everyone can just run phpstan analyse and proper config will be used without the need to pass it as a CLI option. It is especially helpful for setting parallelisation params or editorUrl that enables navigation from CLI to IDE and when you use Docker (so each dev can set it as needed).

@stof
Copy link
Member

stof commented Dec 1, 2023

As our baseline is not big enough so that parsing the neon files becomes the botteneck of running the analysis, I would suggest keeping it in the default neon format to make things easier for contributors so that --regenerate-baseline works out of the box.

Even if we add composer scripts, this won't solve it for contributors that don't know about them and run the phpstan command they know.

@stof
Copy link
Member

stof commented Dec 1, 2023

Regarding strict rules and bleeding edge, I'm not OK to just enable them with everything in the baseline.
The question then is how much of this is fixable (considering that we still need to support multiple PHP versions and that we do have legitimate use cases for == for instance). But some of those errors are indeed worth fixing (for instance the bunch of Token implementations that document their scoring method as returning bool|int instead of the int|false return type of the interface)

@Wirone
Copy link
Contributor Author

Wirone commented Dec 1, 2023

OK, I'll remove strict types and bleeding edge and will provide them in separate PRs with fixes. And will revert NEON format of the baseline 馃憤.

@Wirone Wirone force-pushed the codito/phpstan-enhancements branch from 7eaef75 to 96fbb07 Compare December 1, 2023 15:57
@Wirone
Copy link
Contributor Author

Wirone commented Dec 1, 2023

As our baseline is not big enough so that parsing the neon files becomes the botteneck of running the analysis (...)

@stof PHP format was introduced mostly because of NEON performance, but I pointed other argument in favour of it (in the commit's message) - it's possible to navigate from baseline to the file containing error in a single click in IDE. Nice little perk 馃槈.

@stof
Copy link
Member

stof commented Dec 1, 2023

@Wirone then try to convince the phpstan maintainer to make the PHP format a first-class citizen for --regenerate-baseline called without argument, so that it would detect the format of the existing baseline. Once that is the case, it would remove my objection.

@Wirone Wirone mentioned this pull request Dec 1, 2023
@stof stof merged commit f560293 into phpspec:master Dec 4, 2023
8 checks passed
@Wirone Wirone deleted the codito/phpstan-enhancements branch December 4, 2023 08:24
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.

None yet

2 participants