Upgrade slevomat/coding-standard to v6 #144

simPod · 2019-12-05T10:46:05Z

This aggregates:

Things to note:

There are few traversableTypehints listed for type hints related sniffs. Originally, there was only Doctrine Collection.
Spaces are not required before block control structures now, it's status quo. It's possible to enforce it by disabling excludes for BlockControlStructureSpacing sniff if desired.

Disable TypeHintDeclaration Enable - ParameterTypeHint - PropertyTypeHint - ReturnTypeHint - UselessFunctionDocComment

lcobucci · 2019-12-05T11:12:34Z

As I mentioned to @simPod I'll be taking a look at this to ensure we enforce typed properties on PHP 7.4+

alcaeus · 2019-12-05T11:31:32Z

As I mentioned to @simPod I'll be taking a look at this to ensure we enforce typed properties on PHP 7.4+

Can you elaborate? Will this enforce typed properties if phpcs is run on PHP 7.4, or if the library supports PHP ^7.4?

tests/fixed/type-hints.php

Ocramius · 2019-12-05T11:46:34Z

tests/input/example-class.php

@@ -122,6 +122,7 @@ public function throwWhenInvalid() : void
    public function trySwitchSpace() : void
    {
        try {
+            $var = 1;


This kinda snuck in: not a problem for me as long as it doesn't remove newlines before control structures, but seems unrelated

I just ensured the settings for space before block control structures is not modified from current behaviour. The test for it was missing.

kukulich · 2019-12-05T12:16:19Z

@lcobucci @alcaeus

https://github.com/slevomat/coding-standard/blob/master/SlevomatCodingStandard/Sniffs/TypeHints/PropertyTypeHintSniff.php#L50

That's the default value but it can be changed in configuration: 9578c54#diff-1d794c8c2cf3bf4d562ddaea8be99bb0R349

alcaeus · 2019-12-05T12:23:15Z

Thanks @kukulich! Just wanted to make sure it remains optional.

carusogabriel · 2019-12-05T12:28:28Z

Let's wait to merge this one, we want to perform some functional tests across our repos to see if everything is okay 👍

alcaeus · 2019-12-05T13:00:31Z

@carusogabriel here's ODM: https://gist.github.com/alcaeus/29f2c69ca1444260ea2b976221942c5c

I haven't gone through them, but there are probably some more changes we'll have to take care of. I just made sure that phpcs runs.

simPod · 2019-12-05T13:06:13Z

@alcaeus you should probably upgrade also these excludes https://github.com/doctrine/mongodb-odm/blob/3ac6f4596efce4fb4ff2feb85267d34ddb403c35/phpcs.xml.dist#L28-L29

alcaeus · 2019-12-05T13:13:32Z

@lcobucci can we please split out the patch for PHP 7.4? I have my doubts about the methodology and would rather not have it block this PR. Thanks!

alcaeus · 2019-12-05T13:14:44Z

@simPod thanks for the hint, totally missed those. Gist is updated - most are BlockControlSpacing with a few missing parameter type hints where we either need to add documentation or update the exclude list. At first glance, it looks good 👍

lcobucci · 2019-12-05T13:16:20Z

I have my doubts about the methodology and would rather not have it block this PR. Thanks!

What are your doubts? Could we look at them and discuss before that?

kukulich · 2019-12-05T14:32:21Z

I think that false for native type hints for properties is current state before Slevomat CS 6.0

lcobucci · 2019-12-05T15:41:11Z

Okay, I'm back from my meeting and will try to clarify why having enableNativeTypeHint=false is a no-go from me.

The fact we don't have projects requiring PHP 7.4 is a different story (which is sad to me but I understand the reasons) but despite that this is a library used by other folks and it shouts to everybody (trend setting as @Ocramius keeps saying): ALWAYS DECLARE THE FREAKING TYPES.

The whole Makefile + git patch approach was the easiest way to ensure this message is still propagated (making sure that anyone can checkout this project and run make to test things properly), otherwise build fails on PHP 7.4 (without enableNativeTypeHint=false) for obvious reasons.

alcaeus · 2019-12-05T15:44:14Z

@lcobucci please document the process of maintaining the patch file for contributors and maintainers and I'm good with it. As is, a contributor would have to try and figure out how to fix the build, and I as maintainer would have to guess if I wanted to help them: this isn't how this stuff should be built. As previously stated, I can live with the setting being enabled depending on the PHP versions (especially since this is consistent with other settings) as long as the proper documentation is in place.

lcobucci · 2019-12-05T15:47:22Z

@alcaeus that's completely fair to ask, I'll handle that 👍

kukulich · 2019-12-05T15:48:09Z

@lcobucci I think this PR is about upgrading Slevomat CS and false is compatible with previous behaviour. So it may be better to change the option in different PR

ostrolucky · 2019-12-05T15:48:54Z

All 7.4 builds will be red for PRs targetting our maintained branches (PHP >= 7.2 support), this is why it's bad. But like I said, let's talk about this on different place

lcobucci · 2019-12-05T15:50:19Z

All 7.4 builds will be red for PRs targetting our maintained branches

They won't because it would be a new major release and adjusting code/config to the new version is part of the upgrade.

alcaeus · 2019-12-05T15:58:20Z

All 7.4 builds will be red for PRs targetting our maintained branches (PHP >= 7.2 support), this is why it's bad. But like I said, let's talk about this on different place

To add on to what Luis said, most of our maintained branches run phpcs on the minimum required PHP version (as they should), not on PHP 7.4.

Ensuring that CI also uses the same commands.

Incorporating changes to our tests.

Simplifying what's required from contributors to help us in this project.

lcobucci · 2019-12-05T23:20:33Z

@alcaeus documentation added and make targets are now a bit more resilient.

alcaeus

LGTM. The compatibility patch tooling is neat, thank you for that!

@lcobucci

Handled by @lcobucci's additions

simPod added 3 commits December 5, 2019 11:31

Require slevomat/coding-standard v6

d3e80a3

Crumble removed TypeHintDeclaration sniff

9578c54

Disable TypeHintDeclaration Enable - ParameterTypeHint - PropertyTypeHint - ReturnTypeHint - UselessFunctionDocComment

Do not require space before control structures

86bce23

simPod requested a review from a team as a code owner December 5, 2019 10:46

This was referenced Dec 5, 2019

Require slevomat/coding-standard v6 #142

Closed

Crumble removed TypeHintDeclaration sniff #139

Closed

ostrolucky previously approved these changes Dec 5, 2019

View reviewed changes

simPod mentioned this pull request Dec 5, 2019

Do or do not require spaces before control structures? #138

Closed

alcaeus reviewed Dec 5, 2019

View reviewed changes

tests/fixed/type-hints.php Show resolved Hide resolved

alcaeus previously approved these changes Dec 5, 2019

View reviewed changes

Ocramius reviewed Dec 5, 2019

View reviewed changes

Ocramius previously approved these changes Dec 5, 2019

View reviewed changes

Ocramius added Improvement release blocker labels Dec 5, 2019

Ocramius added this to the 7.0.0 milestone Dec 5, 2019

carusogabriel previously approved these changes Dec 5, 2019

View reviewed changes

lcobucci dismissed stale reviews from carusogabriel, Ocramius, alcaeus, and ostrolucky via 07d785f December 5, 2019 13:08

lcobucci dismissed stale reviews from carusogabriel and themself via 56d128f December 5, 2019 23:05

lcobucci added 5 commits December 6, 2019 00:14

Use a Makefile instead of script to run tests

99b30f7

Ensuring that CI also uses the same commands.

Test PHPCBF behaviour on PHP 7.4+ as well

78c6547

Enforce native types for PHP 7.4+

f9d89e0

Incorporating changes to our tests.

Add make target to generate compatibility patches

76c12e1

Simplifying what's required from contributors to help us in this project.

Update documentation to mention GNU make targets

c53c4bd

lcobucci requested review from alcaeus, ostrolucky and carusogabriel December 5, 2019 23:20

lcobucci approved these changes Dec 5, 2019

View reviewed changes

carusogabriel approved these changes Dec 6, 2019

View reviewed changes

alcaeus approved these changes Dec 6, 2019

View reviewed changes

Ocramius approved these changes Dec 7, 2019

View reviewed changes

Ocramius merged commit 9056d24 into doctrine:master Dec 7, 2019

Ocramius self-assigned this Dec 7, 2019

simPod deleted the upgrade-slevomatcs branch December 7, 2019 09:41

carusogabriel mentioned this pull request Dec 7, 2019

Use stable release of slevomat/coding-standard #140

Closed

alcaeus mentioned this pull request Feb 5, 2020

Enhancement: Add workflow for GitHub Actions #153

Merged

1 task

greg0ire mentioned this pull request Jun 22, 2020

The php72 test-report job is always successful #209

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade slevomat/coding-standard to v6 #144

Upgrade slevomat/coding-standard to v6 #144

simPod commented Dec 5, 2019 •

edited

lcobucci commented Dec 5, 2019

alcaeus commented Dec 5, 2019

Ocramius Dec 5, 2019

simPod Dec 5, 2019

kukulich commented Dec 5, 2019

alcaeus commented Dec 5, 2019

carusogabriel commented Dec 5, 2019

alcaeus commented Dec 5, 2019 •

edited

simPod commented Dec 5, 2019

alcaeus commented Dec 5, 2019

alcaeus commented Dec 5, 2019

lcobucci commented Dec 5, 2019

kukulich commented Dec 5, 2019

lcobucci commented Dec 5, 2019

alcaeus commented Dec 5, 2019

lcobucci commented Dec 5, 2019

kukulich commented Dec 5, 2019

ostrolucky commented Dec 5, 2019 •

edited

lcobucci commented Dec 5, 2019 •

edited

alcaeus commented Dec 5, 2019

lcobucci commented Dec 5, 2019

alcaeus left a comment

Upgrade slevomat/coding-standard to v6 #144

Upgrade slevomat/coding-standard to v6 #144

Conversation

simPod commented Dec 5, 2019 • edited

lcobucci commented Dec 5, 2019

alcaeus commented Dec 5, 2019

Ocramius Dec 5, 2019

Choose a reason for hiding this comment

simPod Dec 5, 2019

Choose a reason for hiding this comment

kukulich commented Dec 5, 2019

alcaeus commented Dec 5, 2019

carusogabriel commented Dec 5, 2019

alcaeus commented Dec 5, 2019 • edited

simPod commented Dec 5, 2019

alcaeus commented Dec 5, 2019

alcaeus commented Dec 5, 2019

lcobucci commented Dec 5, 2019

kukulich commented Dec 5, 2019

lcobucci commented Dec 5, 2019

alcaeus commented Dec 5, 2019

lcobucci commented Dec 5, 2019

kukulich commented Dec 5, 2019

ostrolucky commented Dec 5, 2019 • edited

lcobucci commented Dec 5, 2019 • edited

alcaeus commented Dec 5, 2019

lcobucci commented Dec 5, 2019

alcaeus left a comment

Choose a reason for hiding this comment

simPod commented Dec 5, 2019 •

edited

alcaeus commented Dec 5, 2019 •

edited

ostrolucky commented Dec 5, 2019 •

edited

lcobucci commented Dec 5, 2019 •

edited