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

PSR2 #2

Closed
JBlond opened this issue Feb 20, 2019 · 18 comments
Closed

PSR2 #2

JBlond opened this issue Feb 20, 2019 · 18 comments
Labels
coding style Coding style problem question Just asking some questions

Comments

@JBlond
Copy link

JBlond commented Feb 20, 2019

There are some code violations to PSR2

You use phpcs.xml with

<?xml version="1.0" encoding="UTF-8"?>
<ruleset name="php-diff">
	<description>php-diff</description>
	<rule ref="PSR2" />
</ruleset>

example

php phpcs.phar --standard=phpcs.xml -s -p --colors ./src/
@jfcherng
Copy link
Owner

jfcherng commented Feb 20, 2019

!!

I thought php-cs-fixer would take care that for me with proper settings. But I looks like it doesn't. Thanks for showing me this.

It looks like all errors are about my multi-condition if style like
image

Should be easy fix. 👍

@JBlond
Copy link
Author

JBlond commented Feb 20, 2019

.W....E....E..W.....E.. 23 / 23 (100%)



\src\DiffHelper.php
--------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------------------------------
 87 | WARNING | Line exceeds 120 characters; contains 140 characters (Generic.Files.LineLength.TooLong)
--------------------------------------------------------------------------------------------------------


\src\Renderer\Html\AbstractHtml.php
-------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
-------------------------------------------------------------------------------------------------------------------------------------------------
  63 | ERROR | [x] Expected 0 spaces after opening bracket; newline found (PSR2.ControlStructures.ControlStructureSpacing.SpacingAfterOpenBrace)
 100 | ERROR | [x] Expected 0 spaces after opening bracket; newline found (PSR2.ControlStructures.ControlStructureSpacing.SpacingAfterOpenBrace)
 115 | ERROR | [x] Expected 0 spaces after opening bracket; newline found (PSR2.ControlStructures.ControlStructureSpacing.SpacingAfterOpenBrace)
-------------------------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------------------------------------------------------------------------


\src\Renderer\Html\LineRenderer\Line.php
------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
------------------------------------------------------------------------------------------------------------------------------------------------
 60 | ERROR | [x] Expected 0 spaces after opening bracket; newline found (PSR2.ControlStructures.ControlStructureSpacing.SpacingAfterOpenBrace)
 70 | ERROR | [x] Expected 0 spaces after opening bracket; newline found (PSR2.ControlStructures.ControlStructureSpacing.SpacingAfterOpenBrace)
------------------------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------------------------------------------------------------------


\src\Renderer\Html\LineRenderer\Word.php
--------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------------------------------
 34 | WARNING | Line exceeds 120 characters; contains 127 characters (Generic.Files.LineLength.TooLong)
--------------------------------------------------------------------------------------------------------


\src\Renderer\Text\Unified.php
------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
------------------------------------------------------------------------------------------------------------------------------------------------
 51 | ERROR | [x] Expected 0 spaces after opening bracket; newline found (PSR2.ControlStructures.ControlStructureSpacing.SpacingAfterOpenBrace)
 58 | ERROR | [x] Expected 0 spaces after opening bracket; newline found (PSR2.ControlStructures.ControlStructureSpacing.SpacingAfterOpenBrace)

@jfcherng
Copy link
Owner

jfcherng commented Feb 20, 2019

PSR-2 overview

Expected 0 spaces after opening bracket

It turns out to be there are many people using PHPCS having questions about the Opening parentheses for control structures MUST NOT have a space after them, and closing parentheses for control structures MUST NOT have a space before. rule as stated in the PSR-2. What's the term "space" exactly means?

After the following readings, I currently decide NOT to change my coding style about multiline ifs since I think it's more readable (well, personally). Or, what's the correct coding style for the following multiline if if you must write them in multilines?

if (
    $a &&
    $b &&
    $c &&
    $d 
) {
    // ...
}

I don't think the following one is more readable, seriously.

if ($a &&
    $b &&
    $c &&
    $d) {
    // ...
}

References

Line exceeds 120 characters

They are indeed too long because they evolves long URLs in comments.

PSR-2 states There MUST NOT be a hard limit on line length; the soft limit MUST be 120 characters; lines SHOULD be 80 characters or less.. There is no hard limit on line length so it's fine but I am willing to fix them if it's easy (and not ugly).

@jfcherng jfcherng added the question Just asking some questions label Feb 20, 2019
@jfcherng
Copy link
Owner

jfcherng commented Feb 20, 2019

Thanks for bringing phpcs.

@JBlond
Copy link
Author

JBlond commented Feb 20, 2019

The one in \src\Renderer\Html\LineRenderer\Word.php line 34 is left ;) 7 characters too long. But I can live with that.

@JBlond JBlond closed this as completed Feb 20, 2019
@JBlond
Copy link
Author

JBlond commented Feb 20, 2019

The rule that you excluded could like in Unified.php Could be fixed like

 if ($tag === SequenceMatcher::OP_REP ||
                    $tag === SequenceMatcher::OP_DEL
                ) {
                    $ret .= $this->renderContext('-', $this->diff->getA($i1, $i2));
                }

                if ($tag === SequenceMatcher::OP_REP ||
                    $tag === SequenceMatcher::OP_INS
                ) {
                    $ret .= $this->renderContext('+', $this->diff->getB($j1, $j2));
                }

@JBlond JBlond reopened this Feb 20, 2019
@JBlond
Copy link
Author

JBlond commented Feb 20, 2019

psr2

@jfcherng
Copy link
Owner

jfcherng commented Feb 20, 2019

But it doesn't make sense.

PSR-2 states "Opening parentheses for control structures MUST NOT have a space after them, and closing parentheses for control structures MUST NOT have a space before."

image
In your screenshot, if it's not allowed for a opening parenthesis, why it's "okay now" for a closing parentheses?

@JBlond
Copy link
Author

JBlond commented Feb 20, 2019

Because the first condition after the ( bracket, there is no space. if ($tag expected.. While
if ([EVIL_SPACE OR RETURN]$tag is not correct.

@jfcherng
Copy link
Owner

jfcherng commented Feb 20, 2019

I could accept that the opening bracket is evil.
But in that case, isn't the following evil too if spaces before a closing bracket is not allowed?
image

@JBlond
Copy link
Author

JBlond commented Feb 20, 2019

That is okay, I tested it against the phpcs.xml without the excluded rule. Not saying that it is understandable.

@jfcherng
Copy link
Owner

jfcherng commented Feb 20, 2019

That is a weird one that causing so many people having problems on it unfortunately :/

Thanks god that if ( is 4-char which is the same of a indentation.
But not that fortunate for while (.

while ($a &&
    $b &&
    $c
) {
    // ...
}

is just ugly.

@JBlond
Copy link
Author

JBlond commented Feb 20, 2019

PSR is not the only way to format code ;)

./vendor/bin/phpcs -i
The installed coding standards are MySource, PEAR, PSR1, PSR12, PSR2, Squiz and Zend

@jfcherng
Copy link
Owner

jfcherng commented Feb 20, 2019

PSR-12 is not accepted yet.
But a good thing to know is https://github.com/php-fig/fig-standards/blob/master/proposed/extended-coding-style-guide.md#51-if-elseif-else. 👍

I wish I could follow PSR as much as possible since it's the most well-known currently.
Unofficial though.

@jfcherng jfcherng added the coding style Coding style problem label Feb 20, 2019
@JBlond
Copy link
Author

JBlond commented Feb 22, 2019

@jfcherng it could be worse :D

worse

@jfcherng
Copy link
Owner

jfcherng commented Feb 22, 2019

I dont think your screenshot could even pass PSR-2.

Any way, my codes would pass PSR-12's multiline if and that's enough.
People who want to make PRs with codes like the your screenshot would not be accepted here.
As for other people's libraries, not my business.

@JBlond
Copy link
Author

JBlond commented Feb 28, 2019

For sure is doen't pass PSR2, it was a joke.

@JBlond JBlond closed this as completed Feb 28, 2019
@jfcherng
Copy link
Owner

Just a FYI. PSR-12 is adopted now, which resolves the argument in this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
coding style Coding style problem question Just asking some questions
Projects
None yet
Development

No branches or pull requests

2 participants