phpstan / phpstan-strict-rules

Extra strict and opinionated rules for PHPStan
MIT License
607 stars 48 forks source link

Allow 'make' to work first time #229

Closed fredden closed 1 year ago

fredden commented 1 year ago

I am looking into a suspected bug, so I cloned this repository to review the code. I wanted to check that the tests worked locally on the main branch before trying to make any changes. However, when I ran make, I got errors. The changes in this pull request allow make to work first time.

ondrejmirtes commented 1 year ago

Hi, I'd need to understand these advanced Makefile features and I'm not sure if I want to 😊

fredden commented 1 year ago

Putting things after the rule lists their dependencies. A dependency can either be another target, or a file. You're already using this for the 'check' line: https://github.com/phpstan/phpstan-strict-rules/blob/aeaa0229d0a25051f0aad40164571f7beaf58924/Makefile#L2

The changes here add two new targets, and list them as dependencies where relevant.

Running the cs target before running cs-install results in an error (because the files don't exist). The same is the case for lint or tests where the commands in vendor/bin/ do not yet exist.

ondrejmirtes commented 1 year ago

Thank you, but:

1) What the composer.json target does? vendor: composer.json composer.lock 2) I don't want to run cs-install every time when I run make cs 3) Is it a coincidence or on purpose that vendor target has the name of the vendor directory? 4) I think that generally make can work on Windows, but I'm pretty sure this syntax breaks that: test -f composer.lock 5) I'd want for make to work the same across all repositories in https://github.com/phpstan and I'm not sure if you're going to update all of them. These updates are also not the priority for me right now so I don't know if I'm going to review them and merge them in timely manner. 6) The flow of vendor isn't ideal in some scenarios - for example for the first run it's going to run composer update and composer install right afterwards.

Because of these many issues and questions and also because I'm not a fan of complexity in the Makefile, I decided not to merge this PR. I work on these projects daily and in my opinion the current experience is okay.

fredden commented 1 year ago

Thanks for taking the time to consider this and for explaining your perspective. I can address all of these issues/questions, but don't want to burden you any further.