Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

php-cs-fixer is not properly pinned #598

Open
erayd opened this issue Nov 12, 2019 · 6 comments
Open

php-cs-fixer is not properly pinned #598

erayd opened this issue Nov 12, 2019 · 6 comments

Comments

@erayd
Copy link
Collaborator

erayd commented Nov 12, 2019

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
Copy link
Collaborator Author

erayd commented Nov 12, 2019

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

@SignpostMarv
Copy link
Contributor

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

@erayd
Copy link
Collaborator Author

erayd commented Nov 12, 2019

@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
Copy link
Contributor

@erayd

Can you give an example?

@SignpostMarv
Copy link
Contributor

SignpostMarv commented Nov 12, 2019

@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
Copy link
Collaborator Author

erayd commented Nov 13, 2019

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants