Coding standards rulesets setup with documentation #5
Conversation
denniscoorn-paqt
changed the title
| "nunomaduro/larastan": "^0.6 || ^0.7 || ^1.0 || ^2.0", | ||
| "squizlabs/php_codesniffer": "^3.0", | ||
| "phpmd/phpmd": "^2.0 || dev-master", | ||
| "phpstan/phpstan": "^0.12.50 || ^1.0", |
There was a problem hiding this comment.
Zouden we Larastan niet zelf lekker laten moeten kiezen welke versie van PHPStan hij wil? 🤔
There was a problem hiding this comment.
hm, misschien conflicts hier voor gebruiken? Zodat de coding standaard niet kan conflicteren met de gekozen versie in de applicatie? Zo werkt php-cs-fixer lager dan versie 3 niet met de coding standaard, want daar is het niet .php_cs
There was a problem hiding this comment.
Ik zie dit meer als: wij gebruiken geen PHPStan. Wij gebruiken Larastan. Dus laat Larastan maar zelf bepalen wat hij nodig heeft (en toevallig is dat PHPStan)
There was a problem hiding this comment.
het kan weg ja omdat het geen harde dependency is, maar je kan met conflicts in composer aangeven dat de configuratie die hierin staat niet compatible is met bepaalde versies van een library. Bijv.
"conflicts": {
"squizlabs/php_codesniffer": "<3.0"
}Dan moet de applicatie eerst squizlabs/php_codesniffer naar versie 3 of hoger geupgrade worden voordat de coding standards repo binnengehaald/geupgrade kan worden. Als je geen squizlabs/php_codesniffer in je applicatie hebt wordt het ook niet geïnstalleerd
Het alternatief is mooie foutmeldingen als configuration "setting X" not found en deze zelf uitzoeken hoe je die oplost.
There was a problem hiding this comment.
hm, ik zie nu dat het trouwens require-dev is. Dev dependencies van dependencies worden sowieso niet meegenomen. Maar op dit moment worden de dev dependencies helemaal niet gebruikt, want er is helemaal geen php code.
There was a problem hiding this comment.
er zijn geen dependencies @denniscoorn-paqt , want de hele repository heeft geen php code. Maar wat @johan-w2w zegt klopt ook niet, want dev dependencies van dependencies worden genegeerd in de applicatie.
Waar ik meer aan denk is dat we misschien wel conflicts moeten gebruiken in de composer zodat je bijv. wel je phpmd installatie up to date is bij updaten van de coding standaard.
- Bijv. coding standaard bevat php-cs-fixer 3.0+ configuratie en niet lager, dus conflicts php-cs-fixer lager dan versie 3.
- Stel phpmd gebruikt configuratie optie van deze PR: Add allow-underscore option for CamelCaseParameterName & CamelCaseVariableName phpmd/phpmd#749, dan moet phpmd minstens versie 2.9 zijn om de configuratie te gebruiken (enkel voorbeeld, geen standaard)
zie https://getcomposer.org/doc/04-schema.md#conflict
Op de php-cs-fixer 3.0+ restrictie na, gebruiken we op dit moment niet veel nieuwe configuratieopties.
There was a problem hiding this comment.
Conflicts zou inderdaad een mogelijkheid kunnen zijn om te communiceren wat de restricties zijn. Het is alleen een passieve manier van 'communiceren' wat niet mijn voorkeur zou hebben.
Ik ben het alleen niet met je eens dat je alléén maar dependencies kan definiëren als er ook code in de repository zit. Het doel wat hier bereikt wordt is mijn inziens namelijk vrij duidelijk en volledig legitiem als het gaat over dependency management, namelijk dat: de package paqtcom/coding-standards rulesets aanbied voor verschillende tools en tevens de geschikte dependencies definieert voor die betreffende tools. De noodzaak voor het per project afzonderlijk toevoegen en beheren van de losse dependencies is hierdoor niet meer nodig en het opvoeren van enkel paqtcom/coding-standards is dan voldoende.
Daarnaast bestaat er wel degelijk een correlatie tussen de ruleset en de dependencies naar de afzonderlijke tools. Een ruleset kan bijvoorbeeld wel compatible zijn voor v2 van een tool, maar niet voor v3. Door de rulesets én dependencies op deze manier bij elkaar te zetten kunnen we een correct werking tussen de 2 garanderen. Nieuwe versies van tools en/of nieuwe versies van ruleset betekend in de praktijk een nieuwe versie van deze package wat dan ook op de juiste manier de impact (major, minor of patch) van de verandering uitdraagt.
There was a problem hiding this comment.
Dennis, ik draai bijvoorbeeld alle php tools behalve phpstan buiten de applicatie om via makefiles juist om niet dependencies binnen te halen die niet nodig zijn voor de applicatie. Volgens mij installeren sommige teams het zelfs met composer global.
Als het een dependency wordt krijg ik in vendor/bin opeens wel tools staan die ik niet op die manier aanroep. Hetzelfde geldt als ik niet alle tooling wil gebruiken in een applicatie of composer package. Ik krijg dan wel opeens de tooling mee.
Maar nu zijn het dev dependencies en die zijn helemaal niet nodig, want composer negeert deze dependencies in een applicatie composer.json en er zal ook geen versie compatibility check gedaan worden. conflicts heeft het meeste wenselijke effect
There was a problem hiding this comment.
Oh, dat is wel een goed punt Pieter. Vroeger hadden wij deze tools ook in onze dependencies zitten van de projecten en dit zorgde bij problemen bij het updaten. Sindsdien hebben we alle losgetrokken (op Larastan na), om juist problemen te voorkomen.
There was a problem hiding this comment.
@pieterw2w @johan-w2w Thanks voor de feedback. Het probleem wat geïllustreerd wordt en de mogelijke version constraint issues die kunnen ontstaan bij het updaten van dependencies is inderdaad een bekend probleem. Ik ben mij er ook van bewust dat niet elk team op dezelfde manier het binnen halen van de development dependencies heeft ingericht. Wat ik eerder al heb laten weten hecht ik onder andere veel waarde aan een soepele developer experience en zo min mogelijk discrepantie in tooling tussen developers onderling (van hetzelfde team) en dan is het helpend als er zo min mogelijk 'moving parts' of project setup stappen nodig zijn. Elke oplossingsrichting heeft zijn eigen voor- en nadelen hebben en alles afwegende dacht ik oprecht dat deze universele composer dependencies oplossing een richting was waar uiteindelijk iedereen zich wel in zou kunnen vinden.
Dat gezegd hebbende denk ik dat er wel een alternatieve oplossingsrichting is die iets meer tegemoet zou kunnen komen aan de nadelen van deze oplossing zoals die hierboven beschreven staan. Ik zal daarmee aan de slag gaan en binnen niet al te lange tijd een nieuwe PR voor aanmaken.
|
PR voor alternatieve oplossing is hier te vinden 👉 #6. |
De config files voor de verschillende coding standard tools zijn nu herbruikbaar. De instructies daarvoor staan in de Readme.