plugin-php icon indicating copy to clipboard operation
plugin-php copied to clipboard

PSR-12 is this properly supported yet?

Open snake-py opened this issue 1 year ago • 6 comments

Hello, I am trying to use this with psr-12 standard, but I think I am doing something wrong. I want to use this plugin before PHP_CodeSniffer, which uses the PSR-12 standard. I found several threads already speaking about it, but the I did not find anything in the documentation.

Can you help me to set this up?

# my script

#! /usr/bin/env bash

run_prettier(){
files=$1  
 if [ "$files" != "" ]
then
  echo "Running Prettier... on $files"
  prettier --write $files
  if [ $? != 0 ]
  then
    echo "Fix the error before commit."
    exit 1
  else
    # Add back the modified/prettier files to staging
    echo "$files" | xargs git add
  fi
fi
}

# output
Running Prettier... on src/LoggingServicePackageProvider.php
[error] Invalid braceStyle value. Expected "1tbs" or "psr-2", but received "psr-12".
Fix the error before commit.

.prettierrc:

{
    "phpVersion": "8.1",
    "printWidth": 80,
    "tabWidth": 4,
    "useTabs": false,
    "singleQuote": false,
    "trailingCommaPHP": true,
    "braceStyle": "psr-12",
    "requirePragma": false,
    "insertPragma": false
}

snake-py avatar Sep 08 '22 12:09 snake-py

@snake-py thanks for bringing this up! In my mind PSR-12 was just an extension on top of PSR-2, but it turns out that PSR-2 has been replaced by PSR-12:

As of 2019-08-10 PSR-2 has been marked as deprecated. PSR-12 is now recommended as an alternative.

https://www.php-fig.org/psr/psr-2/

I think we should rename the braceStyle option to from psr-2 to psr-12, since 12 defines exactly the same brace style convention (as far as I can tell). To avoid the breaking change, we can keep the psr-2 as a synonym, but remove it from the docs. What do you think @cseufert?

@snake-py For your case, you'll probably get what you expect by specifying "braceStyle": "psr-2".

czosel avatar Sep 13 '22 17:09 czosel

Well the issue I am facing is if I run prettier on my files and afterward I run PHP_CodeSniffer the changes prettier made will collide with the PSR-12 standard. This makes the package for me unusable. I thought if I used the same standard everything should be formatted the same.

I can see if I can make an example where I run into issues, but my lead is already annoyed because I spend a lot of time on this :D

@czosel I am unsure if this will be out of scope, but is there a way how to format the code? Can I define the amount of character in one line (when to break etc. ?) In all honesty, this is the first time that I use prettier from the command line :D and even have to set it up.

snake-py avatar Sep 13 '22 18:09 snake-py

@snake-py Thanks for clarifying, I misunderstood your original post. Could you share some examples how the output of our plugin is in conflict with PSR-12?

I'm not using PHP_CodeSniffer myself, but if it's comparable to eslint this might call for a package like https://github.com/prettier/eslint-config-prettier which disables all the linting rules that are in conflict with Prettier?

Concerning line width: The Prettier core option --print-width should have you covered. You can play with the effect in our playground: https://loilo.github.io/prettier-php-playground/

czosel avatar Sep 14 '22 12:09 czosel

@czosel for instance:

                foreach ($client->getContractsByCustomerId($customer["id"]) as $contract) {

prettier will do this:

                foreach (
                    $client->getContractsByCustomerId($customer["id"])
                    as $contract
                ) {

I will now get the following error from code sniffer with PSR12

FILE: /app/app/Services/AuthService.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 388 | ERROR | [x] Expected 1 space before "as"; 20 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

snake-py avatar Sep 20 '22 09:09 snake-py

@snake-py I would see this as more of a bug on the PHPCBF ruleset, as it is not really violating any specific PSR-12 rule that I can see, however it does not appear to know how to deal with a foreach expression split over multiple lines.

cseufert avatar Sep 21 '22 03:09 cseufert

I think we should rename the braceStyle option to from psr-2 to psr-12, since 12 defines exactly the same brace style convention (as far as I can tell). To avoid the breaking change, we can keep the psr-2 as a synonym, but remove it from the docs. What do you think @cseufert?

Great idea, I have opened a PR for this already.

cseufert avatar Sep 21 '22 04:09 cseufert

PSR-12 editor here, just want to give you a heads up that PSR-12 is now stale and the Coding Style PER v1.0.0 should be used in its place moving forward. The value of relying on the CS-PER instead of PSR-12 is the CS-PER will be releasing new versions that add support for new language features as they are added and PSR-12 will likely be deprecated soon once we get out the next version of CS-PER. You can see the latest version of CS-PER (Which is just PSR-12 with names changed) here: https://www.php-fig.org/per/coding-style/

KorvinSzanto avatar Sep 26 '22 19:09 KorvinSzanto

@KorvinSzanto Thanks for the heads-up! @cseufert Should we call it PER then? 🙈

czosel avatar Sep 26 '22 19:09 czosel

@czosel Yes, I think aliasing PSR-2, to PER or PER-CS would make sense, I'm leaning more to PER-CS as I assume there will be additional PER's in the future

cseufert avatar Sep 26 '22 23:09 cseufert

Hey, I opened a clarification ticket on the PSR12 guide about this and they told me that it is according to their standard "not okay" to break foreach up into multiple lines https://github.com/php-fig/per-coding-style/issues/49#issuecomment-1258509807

Is there a way to prevent line breaks for certain expressions?

snake-py avatar Sep 27 '22 07:09 snake-py

Yes, in principle we can control the line breaks. But in this case I'm not sure if it's the correct way to go. We have always used PSR / PER as guidance when making formatting decisions, but we never aimed to be fully PSR / PER compliant. Probably there are many more examples for this. To me, the idea behind this plugin is to stay reasonably close to how Prettier for JS works. What do you think @cseufert?

czosel avatar Sep 27 '22 10:09 czosel

I feel that trying to control the behaviour in these edge cases goes against the spirit of prettier itself My reasons against:

  • The PER-CS group suggest refactoring the loop to use a temp variable, which sounds reasonable.
  • Prettier tries hard not to exceed the max line length, which I feel is one of the most important aspects, to keep code readable, especially diff/patches.

cseufert avatar Sep 27 '22 10:09 cseufert

@czosel I mean no offense, but this will make the plugin effectively useless to me, and probably other people as well.

@cseufert

The PER-CS group suggest refactoring the loop to use a temp variable, which sounds reasonable.

sure this will solve this particular issue, however it will not solve the overall issue..

Prettier tries hard not to exceed the max line length, which I feel is one of the most important aspects, to keep code readable, especially diff/patches.

I am unsure how complex this would be and I also would be willing to code it myself, with some guidance. But can we not have a simple setting that accepts an array, which will disable line breaks for this particular line?

Something like:

{
"disableLineBreaksOnKeywords": ["foreach"]
}

snake-py avatar Sep 27 '22 11:09 snake-py

Any further thoughts regarding this issue or do you just want to stick to the statement that this package is not written with the purpose to fulfill any coding standard?

snake-py avatar Oct 06 '22 18:10 snake-py

I agree with @cseufert‘s statement about line length. Also we’re very conservative about adding new options as explained in Prettiers Option Philosophy.

Personally, very specific style guides have become irrelevant to me since I started using prettier. Prettier aims to format code in a way that is readable, as if it had been manually formatted by the developer, but in an automatic and reproducible way. To me, style guides are useful to define how Code in a specific project should be formatted, if no automatic formatter is available. With prettier this is implicitly defined, making a sort of „automatic style guide“. The fact that it doesn’t match any other manual style guide is not important anymore.

What’s the reason you feel that you have to be 100% PER-compliant @snake-py ?

czosel avatar Oct 06 '22 18:10 czosel

What’s the reason you feel that you have to be 100% PER-compliant

Well, because our code quality check in our pipeline uses PSR-12 standards. The code quality check in our pipelines is done by code climate and it checks more than just formatting like it checks how big functions are and if there are strings that should be used as constants etc. But it also checks if everything is formatted to PSR-12. I am not sure if I can disable the formatting check.

But I understand your argument and I will see how I want to solve this. Either I will drop prettier and try PHPCBF again or I will try to disable the formatting checks inside the pipeline.

snake-py avatar Oct 07 '22 06:10 snake-py

Right - I think in the long run we'll need a package similar to prettier-config-eslint that disables all the formatting rules that conflict with Prettier. See also: https://prettier.io/docs/en/integrating-with-linters.html

czosel avatar Oct 07 '22 08:10 czosel

But I understand your argument and I will see how I want to solve this. Either I will drop prettier and try PHPCBF again or I will try to disable the formatting checks inside the pipeline.

If you do figure out what checks conflict with prettier and how to disable them, please consider documenting these for other prettier users.

cseufert avatar Oct 09 '22 22:10 cseufert