Add doctrine/conding-standard as phpcs rule #210

DanielBadura · 2020-10-10T20:12:08Z

This PR aims to get a coding standard into the project to improve readability and maintainability. I chose doctrine/coding-standard but if wanted we can switch the cs rules or require slevomat/coding-standard or squizlabs/php_codesniffer directly. Also adding / removing rules should be no problem.

At the moment I only installed the package and let phpcbf auto fix the issues that could be fixed. But there are plenty more cs issues to work on.

But before i work more on this i would get your opinion on this topic @Ocramius @maglnet :)
I would also add it to the CI and maybe remove the scrutinizer config.

…oding standard

Ocramius · 2020-10-10T20:17:18Z

If the diff is really all there is to it, it seems like we're already quite in line with it anyway :-)

DanielBadura · 2020-10-10T20:52:20Z

No the diff is only that stuff that could be auto fixed. There is alot more todo. I will add the CI job as a next step :)

DanielBadura · 2020-10-10T21:09:09Z

Well I added a config file now and added phpcs to the CI. Now i got some more autofixes and some tests are crashing now. And alot of issues are still open in phpcs. So it'll be alot of work. But for tomorrow :D

…ector::recordFunctionParameterTypesUsage due to phpcbf autofix to an earlyout that let to false types

DanielBadura · 2020-10-12T07:50:41Z

Tests are fixed now. It was just a little thing.. 🥳
But there are still many cs fixes open.

DanielBadura · 2020-10-18T17:15:55Z

@Ocramius this patch is almost ready. There are still 3 warnings open. I dont really know how to solve this with the line length and the arrays since i cannot create a newline with only one value in the array. :D

Ocramius · 2020-10-18T18:36:00Z

We could disable that rule: it ain't vital

DanielBadura · 2020-10-19T09:51:14Z

Okay now the patch is ready for review. I excluded now the file too long cs.
So now the pipeline should run green besides of appveyor and scrutinizer. I removed the scrutinizer config but its is still running on this PR. So I think this is like appveyor as a config in GitHub?

One bigger change is made here. JsonLoader::getData is now a static method which returns the data directly instead of initiating the JsonLoader with the file path and getting the data separately.

Ocramius · 2020-10-19T10:48:50Z

So now the pipeline should run green besides of appveyor and scrutinizer. I removed the scrutinizer config but its is still running on this PR. So I think this is like appveyor as a config in GitHub?

Yep, we need @maglnet to hop in and shut them down in the repo settings :D

Ocramius

LGTM, but the rename of GuesserInterface to Guesser, as well as the exception names, is a BC break that requires a new major release.

IMO fine, but we'd have to rename 2.2.0 to 3.0.0.

There are also many non-final symbols for which public and protected API changed, so that is also a BC break.

@maglnet I need your review on this, but from a coding perspective it is fine (besides minor comments).

If all is fine, I'd rename the milestone to 3.0.0 and move on from there.

Ocramius · 2020-10-19T12:21:23Z

src/ComposerRequireChecker/JsonLoader.php


+/**
+ * @internal


'bit of a BC break, but IMO acceptable, since this isn't really a library

Ocramius · 2020-10-19T12:42:01Z

src/ComposerRequireChecker/NodeVisitor/UsedSymbolCollector.php

-            )
-            && $node->class instanceof Node\Name
+        if (
+            (! ($node instanceof Node\Expr\StaticCall)


This conditional got much harder to read: a simpler way to change this is to reverse it with a single ! in front of the original version

With this psalm could not get the type right. Now the condition is split in two and is with this IMHO definitely more readable than before. WDYT?

DanielBadura · 2020-10-19T13:32:42Z

I could also revert some of BC's if this helps :)

Ocramius · 2020-10-19T13:33:57Z

IMO they make sense, and the upgrade would be painless for 99.9% of users out there (if we go for 3.0.0): it's just a decision that the repo owner has to take.

…m cannot interfere the type correctly, psalm things its a property check on a interface; Interfaces cannot have properties (see https://psalm.dev/028)

maglnet · 2020-10-29T14:16:20Z

I'm sorry, currently I cannot relly invest time in OSS, sad but true 🙈

I'm totally fine with BC breaking changes for a 3.0.0 release since this does not really affect the CLI API, which is the API for nearly all users of this package.

@Ocramius @DanielBadura
If I understand it correctly: I now disabled AppVeyor and Scruitinizer for this Repository.
Do I need to trigger a new CI-job?

DanielBadura · 2020-10-29T16:06:31Z

No problem at all!

As far as i know: Disabling AppVeyor & Scrutinizer should be all. No need to add a new CI-Job.
But im not so familiar with GitHub's CI Settings.

Ocramius · 2020-10-29T16:21:04Z

@maglnet no worries, it is what it is :-)

DanielBadura · 2020-11-07T20:02:04Z

@Ocramius is here something missing that i could take care of?

Ocramius · 2020-11-08T14:55:48Z

I'm fine with it, will keep this in the next major, and we roll with that 👍

Ocramius · 2020-11-08T14:56:06Z

Thanks, @DanielBadura!

DanielBadura added 2 commits October 10, 2020 21:59

add doctrine coding standard as dev dependency to get a standarzied c…

3609bc0

…oding standard

let phpcbf autofix some cs issues

d245f69

DanielBadura added 3 commits October 10, 2020 22:53

add phpcs as ci job via github workflow

e886a9d

add phpcs config

b5ee0f5

run again auto fix with phpcbf

0da7f0a

fix nullable ErrorHandle was not nullable anymore, fix UsedSymbolColl…

5d9b7ee

…ector::recordFunctionParameterTypesUsage due to phpcbf autofix to an earlyout that let to false types

DanielBadura added 2 commits October 18, 2020 19:07

fix phpcs issues, now only 3 warnings for too long lines are missing

e620866

fix some doctypes

f55aa39

DanielBadura added 3 commits October 19, 2020 11:43

exclude line length too long in phpcs for the missing 3 warnings

4171427

remove scrutinizer config

c24141f

remove scrutinizr badge

6716a50

DanielBadura marked this pull request as ready for review October 19, 2020 09:45

Ocramius approved these changes Oct 19, 2020

View reviewed changes

Ocramius added this to the 3.0.0 milestone Oct 19, 2020

Ocramius added bc break dependencies Pull requests that update a dependency file enhancement help wanted labels Oct 19, 2020

Ocramius assigned maglnet Oct 19, 2020

Ocramius requested a review from maglnet October 19, 2020 12:44

Ocramius mentioned this pull request Oct 19, 2020

Psalm patrol #206

Merged

adjust condition to be more readable, split in two checks due to psal…

8aba1a0

…m cannot interfere the type correctly, psalm things its a property check on a interface; Interfaces cannot have properties (see https://psalm.dev/028)

maglnet approved these changes Oct 29, 2020

View reviewed changes

Ocramius merged commit 427cfc3 into maglnet:master Nov 8, 2020

DanielBadura deleted the add-php-cs branch November 30, 2020 15:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add doctrine/conding-standard as phpcs rule #210

Add doctrine/conding-standard as phpcs rule #210

DanielBadura commented Oct 10, 2020

Ocramius commented Oct 10, 2020

DanielBadura commented Oct 10, 2020

DanielBadura commented Oct 10, 2020

DanielBadura commented Oct 12, 2020

DanielBadura commented Oct 18, 2020

Ocramius commented Oct 18, 2020

DanielBadura commented Oct 19, 2020

Ocramius commented Oct 19, 2020

Ocramius left a comment

Ocramius Oct 19, 2020

Ocramius Oct 19, 2020

DanielBadura Oct 20, 2020

DanielBadura commented Oct 19, 2020

Ocramius commented Oct 19, 2020

maglnet commented Oct 29, 2020

DanielBadura commented Oct 29, 2020

Ocramius commented Oct 29, 2020

DanielBadura commented Nov 7, 2020

Ocramius commented Nov 8, 2020

Ocramius commented Nov 8, 2020


		/**
		* @internal

Add doctrine/conding-standard as phpcs rule #210

Add doctrine/conding-standard as phpcs rule #210

Conversation

DanielBadura commented Oct 10, 2020

Ocramius commented Oct 10, 2020

DanielBadura commented Oct 10, 2020

DanielBadura commented Oct 10, 2020

DanielBadura commented Oct 12, 2020

DanielBadura commented Oct 18, 2020

Ocramius commented Oct 18, 2020

DanielBadura commented Oct 19, 2020

Ocramius commented Oct 19, 2020

Ocramius left a comment

Choose a reason for hiding this comment

Ocramius Oct 19, 2020

Choose a reason for hiding this comment

Ocramius Oct 19, 2020

Choose a reason for hiding this comment

DanielBadura Oct 20, 2020

Choose a reason for hiding this comment

DanielBadura commented Oct 19, 2020

Ocramius commented Oct 19, 2020

maglnet commented Oct 29, 2020

DanielBadura commented Oct 29, 2020

Ocramius commented Oct 29, 2020

DanielBadura commented Nov 7, 2020

Ocramius commented Nov 8, 2020

Ocramius commented Nov 8, 2020