jsonrainbow / json-schema

PHP implementation of JSON schema. Fork of the http://jsonschemaphpv.sourceforge.net/ project
MIT License
3.53k stars 351 forks source link

php-cs-fixer is not properly pinned #598

Open erayd opened 4 years ago

erayd commented 4 years ago

php-cs-fixer is still trying to apply new, breaking changes to the source - in this case it's removing parts of the documentation comments - despite #537 which was explicitly intended to prevent this.

/CC @localheinz

erayd commented 4 years ago

/CC @SignpostMarv also, as you were the last person to touch this in #575.

SignpostMarv commented 4 years ago

Is this regarding that annoying habit php-cs-fixer has of removing non-starred lines in multi-line docblock tags ?

erayd commented 4 years ago

@SignpostMarv Based on a quick look at what it's doing, it seems to be removing argument lines with a technically invalid type - which are deliberately the way they are to be more descriptive (e.g. string|null).

localheinz commented 4 years ago

@erayd

Can you give an example?

SignpostMarv commented 4 years ago

@erayd this seems to be fine. There'll be rules to configure to change the behaviour. However....

@param tags with no description that are directly match the code are redundant.

  10) tests\Constraints\TypeTest.php (no_superfluous_phpdoc_tags, phpdoc_align)
      ---------- begin diff ----------
--- Original
+++ New
@@ -69,8 +69,7 @@
     /**
      * Helper to assert an error message
      *
-     * @param string         $expected
-     * @param TypeConstraint $actual
+     * @param string $expected
      */
     private function assertTypeConstraintError($expected, TypeConstraint $actual)
     {

mid-line indentation is a matter of taste, although additionally I'm going to hazard a guess most of these should be more specific than mixed.

   5) src\JsonSchema\Constraints\UndefinedConstraint.php (no_superfluous_phpdoc_tags, phpdoc_align)
      ---------- begin diff ----------
--- Original
+++ New
@@ -55,10 +55,9 @@
     /**
      * Validates the value against the types
      *
-     * @param mixed       $value
-     * @param mixed       $schema
-     * @param JsonPointer $path
-     * @param string      $i
+     * @param mixed  $value
+     * @param mixed  $schema
+     * @param string $i
      */

As with the single-typed redundant line removal, T|null is a redundant line when the actual parameter matches the specified type, i.e. JsonPointer|null $path

   2) src\JsonSchema\Constraints\Constraint.php (no_superfluous_phpdoc_tags, phpdoc_trim, phpdoc_align)
      ---------- begin diff ----------
--- Original
+++ New
@@ -56,10 +56,9 @@
     /**
      * Validates an array
      *
-     * @param mixed            $value
-     * @param mixed            $schema
-     * @param JsonPointer|null $path
-     * @param mixed            $i
+     * @param mixed $value
+     * @param mixed $schema
+     * @param mixed $i
      */
erayd commented 4 years ago

@SignpostMarv I don't care that they are redundant; I don't want them removed. They will eventually have descriptions (when somebody has the time to finish cleaning up the docs), but in the meantime I still don't want php-cs-fixer removing parameters. Indentation isn't a problem; it seems to be complying with our existing scheme for them.

My main annoyance with this is php-cs-fixer is yet again changing its behavior without warning. That was the whole reason we pinned it in the first place, as in the past some of those changes included logical changes to the code, not just cleanup (i.e. it was making assumptions and changing things in a way that would have caused the validation logic to change and no longer comply properly with the spec). It might be changing "just comments" this time, but it's still making assumptions and changing stuff that it previously left alone - in this case, including the deletion of lines that are there deliberately.