Clarification on "PSR2 conflicts with Readability Best Practices"

638 views
Skip to first unread message

Phil Sturgeon

unread,
Apr 4, 2013, 9:25:43 AM4/4/13
to php...@googlegroups.com
I just got something wrong. Oops.

In regards to the conversation "PSR2 conflicts with Readability Bes" on GitHub (which got silly and rude very quickly, good old internet...) the question ofif ( ! $var) { vrs if (!$var) { was raised. I'd always considered PSR-2 to not explicitly rule on this, and assumed the CS-Fixer was being over-enthusiastic about making this a rule.

Sadly this rule makes it so:

There MUST NOT be a space after the opening parenthesis

It was my understanding that this was to avoid people writing code like if ( ($foo == 'bar' ) and ( false ) ) { for no apparent reason, but this sadly effects "if ( ! $foo" too.

I do not want to argue an equivalent conversation of tabs v spaces, and I've told people to shut up on that front a bazillion times. I am however concerned that this genuinely does detract from readability, and would like to be certain this is what we are telling people to do.

If that was the intention, then that is one thing, but was it? I feel like this might have been move of an oversight to not mention whitespace about bangs, than an explicit ruling that definitely wrapping bangs in whitespace is bad.

If anyone can shed some light on the situation that would be awesome. 

Please do not argue about which you prefer.

Andrew Eddie

unread,
Apr 4, 2013, 11:46:29 PM4/4/13
to php...@googlegroups.com
On Thursday, 4 April 2013 23:25:43 UTC+10, Phil Sturgeon wrote:
I do not want to argue an equivalent conversation of tabs v spaces, and I've told people to shut up on that front a bazillion times. I am however concerned that this genuinely does detract from readability, and would like to be certain this is what we are telling people to do.

I think it's a non-issue because you can argue that about almost every part of the standard (K&R vs Alman braces for example, among others). The problem is "your" genuine issue is another person's "personal preference". Regardless, you are between a rock and a hard place.

If it's a genuine readability issue, the PSR-2 should err on the side of MUST, but that breaks compatibility. 

If it's not (and let's face it, not everyone will agree it is), you might argue for "(!$test)" OR "( ! $test)", but then you open the flood gates for why can't we have OR tabs, and OR Alman braces, and OR this and that.

Or, you just generate a variation on PSR-2 as PSR-N and we simply have two (or more) competing code style standards just like we are going to have two autoloader standards, but for the sake of one minor change like that, it's hardly worth it seeing as PSR-2 only just passed in the first place.

As for me, I'd just say PSR-2 is what it is and ignore the bits that don't suit you.

Regards,
Andrew Eddie

Phil Sturgeon

unread,
Apr 5, 2013, 9:55:23 AM4/5/13
to php...@googlegroups.com
I know I know and I definitely don't want to open up the floodgates on "Tabs OR Spaces" which would be crazy, the reason I ask is that it feels like it was left out in the first place.

My real question is: Was it the intent of the group to rule against if ( ! $foo and allow only if(!, or is it supposed to be personal preference?
  • All PHP files MUST use the Unix LF (linefeed) line ending.
  • All PHP files MUST end with a single blank line.
  • The closing ?> tag MUST be omitted from files containing only PHP.
  • PHP keywords MUST be in lower case.
All very explicit. You MUST do it this way.
  • There MUST be one space after the control structure keyword
  • There MUST NOT be a space after the opening parenthesis
  • There MUST NOT be a space before the closing parenthesis
  • There MUST be one space between the closing parenthesis and the opening brace
  • The structure body MUST be indented once
  • The closing brace MUST be on the next line after the body
These are all incredibly explicit rules and completely negate any room for people's personal opinion, except for whining about it. I'm not doing that, im just saying that the case of "what do we do with a bang in a control statement" to me might potentially be unanswered. 

Some people (including myself) and Ben Ramsey, and apparently some folks on #phpfig https://twitter.com/ramsey/status/319462980712558592 have always thought this is user preference, yet when this explicit rule is combined with the CodeSniffer rules you're going to get errors if you wrap your bang in whitespace.

Perhaps this is simply a conversation we have with the CS team, where we get them to ignore the "There MUST NOT be a space after the opening parenthesis" if there is a bang involved? That seems like it might be a resolution.

Paul Jones

unread,
Apr 5, 2013, 10:40:01 AM4/5/13
to php...@googlegroups.com

On Apr 5, 2013, at 8:55 AM, Phil Sturgeon wrote:

> Perhaps this is simply a conversation we have with the CS team, where we get them to ignore the "There MUST NOT be a space after the opening parenthesis" if there is a bang involved? That seems like it might be a resolution.

I have said this elsewhere, but I'll say it here: this strikes me as an opportunity to revisit the many things we left out of PSR-1 and 2, and structure them as a "best practices/readability/etc" recommendation. I would say there's no harm in saying in that new recommendation that "(! ...)" is an allowed exemption from that one PSR-2 rule.


-- pmj

Andrew Eddie

unread,
Apr 5, 2013, 4:52:40 PM4/5/13
to php...@googlegroups.com
On 6 April 2013 00:40, Paul Jones <pmjo...@gmail.com> wrote:
I have said this elsewhere, but I'll say it here: this strikes me as an opportunity to revisit the many things we left out of PSR-1 and 2, and structure them as a "best practices/readability/etc" recommendation.  I would say there's no harm in saying in that new recommendation that "(! ...)" is an allowed exemption from that one PSR-2 rule.

You've also said that the point of PSR-2 was to show the commonality between member projects. So, if PSR-2 is supposed to be about "best practice" we need to throw it out and start again because if fails miserably. Which one is it?

Regards,
Andrew Eddie

Paul Jones

unread,
Apr 5, 2013, 5:13:42 PM4/5/13
to php...@googlegroups.com
PSR-2 is the result of surveyed commonalities between the projects (not "best practices").

I failed to make explicit that by "revisit" I meant "bring up for discussion and perhaps structure a new recommendation that extends on PSR-2" (which is what I've said in other communications but did not make clear here). There were a lot of things in the original proposal that were left out of PSR-1 and 2. Indeed, many of those things left out were not surveyed; for example, whether or not "return early" is used on a regular basis, whether they prefer "if(! $foo)" or "if( ! $foo)" when using a leading bang, etc etc.

Thus, it may well be reasonable to revisit those things that were left out, survey them, discuss them, and come up with another PSR regarding them.


-- pmj

Andrew Eddie

unread,
Apr 5, 2013, 5:31:37 PM4/5/13
to php...@googlegroups.com
On 6 April 2013 07:13, Paul Jones <pmjo...@gmail.com> wrote:
I failed to make explicit that by "revisit" I meant "bring up for discussion and perhaps structure a new recommendation that extends on PSR-2" (which is what I've said in other communications but did not make clear here).  There were a lot of things in the original proposal that were left out of PSR-1 and 2. Indeed, many of those things left out were not surveyed;  for example, whether or not "return early" is used on a regular basis, whether they prefer "if(! $foo)" or "if( ! $foo)" when using a leading bang, etc etc.

Thus, it may well be reasonable to revisit those things that were left out, survey them, discuss them, and come up with another PSR regarding them.

Ok, I can get behind that.

Regards,
Andrew Eddie 

Phil Sturgeon

unread,
Apr 5, 2013, 7:56:55 PM4/5/13
to php...@googlegroups.com
So there are two threads to this conversation now:

1.) Making a new PSR which adds on top of PSR-2

2.) Pauls comments suggest "if ( !" is not explicitly disallowed by PSR-2

This means pestering the CS team to change the PSR-2 rule to avoid throwing an error if I use "if ( ! " is a valid thing to do, right?

Greg Sherwood

unread,
Apr 6, 2013, 12:21:20 AM4/6/13
to php...@googlegroups.com
On Saturday, 6 April 2013 10:56:55 UTC+11, Phil Sturgeon wrote:
2.) Pauls comments suggest "if ( !" is not explicitly disallowed by PSR-2

This means pestering the CS team to change the PSR-2 rule to avoid throwing an error if I use "if ( ! " is a valid thing to do, right?

It is implicitly disallowed. It is not explicitly or implicitly allowed. Therefore, it is disallowed by the standard.

There is no way for a standard to explicitly allow and disallow every current and future configuration of code into valid syntax. That's something common to all coding standards I've seen. Through working on PHP_CodeSniffer, I've also seen a *lot* of interesting code that I had to actually run through "php -l" just to make sure it really is valid. You can't write a standard to cover that sort of thing. You have to live with implicit rules.

When an exception is discovered, you can then change your rules, or add explicit rules to cover the new cases. A coding standard should be an evolving document, like the language (or even a spoken language). So far, PSR-2 hasn't been changed, but it certainly could if the voting members agree.

The point I really want to make is that PHP_CodeSniffer's implementation of the PSR-2 standard is faithful to line that bans "if ( ! ". It does not matter what the intentions are of the voting members now, or what they were then. It always has to remain faithful to the written document so there is no ambiguity. So I can't just change the PSR-2 standard inside PHP_CodeSniffer without a change to the written document first. Hopefully that makes sense to more people than myself.

Phil Sturgeon

unread,
Apr 6, 2013, 4:23:34 PM4/6/13
to php...@googlegroups.com
Hey Greg,

Thanks for stopping by.

It is implicitly disallowed. It is not explicitly or implicitly allowed. Therefore, it is disallowed by the standard.
 
I'm not sure I agree with this line of logic. Not only have several members of the team in various places state that "if ( ! " or "if (!" is persona preference, but just because something is not mentioned to be allowed does not mean it is banned. 

Just because there is no rule saying "DocBlocks MUST NOT contain ASCII art of a unicorn jumping over a rainbow." doesn't mean im not allowed to decorate my docblocks with that beautiful horned creature.

When an exception is discovered, you can then change your rules, or add explicit rules to cover the new cases. A coding standard should be an evolving document, like the language (or even a spoken language). So far, PSR-2 hasn't been changed, but it certainly could if the voting members agree.

There are three schools of thinking, some people will shout NO CHANGES EVER in your face blindly until they go red, and some believe that new rules can be added for new cases that may have been missed out. I think I might be for the later, and this is something that could fit into that, but im fine with the third school: Making a PSR-4 that adds on top of PSR-2.

Either way, whatever PSR-4 might say (if it ever happens), and wether PSR-2 is changed or not, the fact is that PSR-2 by its definition or by the intentions of most of the original folks who put it together does not say you CANNOT do "if ( ! ". Several people have said it is meant to be allowed as an option, so the fact that CodeSniffer shouts you down if you use this optional (and valid) syntax means that CodeSniffer PSR2 rules need to change.

Right?

Phil Sturgeon

unread,
Apr 6, 2013, 4:25:09 PM4/6/13
to php...@googlegroups.com
To clarify, all im suggesting here is that the PSR-2 CodeSniffer rules will avoid showing an error for the "no spaces after opening parenthesis" rule in the case that the syntax "if ( ! " is used. 

I assume that technically it's possible?

Greg Sherwood

unread,
Apr 6, 2013, 11:19:37 PM4/6/13
to php...@googlegroups.com


On Sunday, 7 April 2013 06:23:34 UTC+10, Phil Sturgeon wrote:
Hey Greg,

Thanks for stopping by.

It is implicitly disallowed. It is not explicitly or implicitly allowed. Therefore, it is disallowed by the standard.
 
I'm not sure I agree with this line of logic. Not only have several members of the team in various places state that "if ( ! " or "if (!" is persona preference, but just because something is not mentioned to be allowed does not mean it is banned.

Agreed, if (and only if) the standard didn't say anything that banned that formatting. But it does. Here is how I would have to code the logic in the sniff itself:

if ($spaceAfter === true && $nextChar !== T_LOGICAL_NOT) {
    $phpcs->addError(...)
}

I have to add an *explicit* check for this type of code because it violates an existing rule of the standard. The fact that it is not mentioned in the standard means I should not be making an exception for it in PHP_CodeSniffer. There is no way of writing the check without explicitly looking for the T_LOGICAL_NOT token type.

I'm not sure what I am missing that causes you to disagree with my logic. I don't even use the PSR2 standard, so I'm not imparting personal preference into my reasoning.

Your comment about how various people have said it is due to personal preference seems irrelevant to me. Only because the standard is so clear on this point. It doesn't actually matter if all the voting members said it was allowed code. Writing code like that directly violates a MUST NOT rule in the standard and the voting members should vote to add an explicit exclusion into PSR2 if it was not the intention of banning this code.
 

Just because there is no rule saying "DocBlocks MUST NOT contain ASCII art of a unicorn jumping over a rainbow." doesn't mean im not allowed to decorate my docblocks with that beautiful horned creature.

Your example doesn't make any sense to me in this context, although it is amusing :)

I think a more relevant example is if the standard said "DocBlocks MUST NOT contain ASCII art of an animal" and then you decided to add an ASCII unicorn to a docblock. No, the standard does not explicitly say that unicorns are banned, and you can argue all day if a unicorn is actually an animal given it is imaginary, but the standard clearly bans your ASCII unicorn jumping over anything in this case. The standard would need to be changed to "DocBlocks MUST NOT contain ASCII art of an animal, unless the animal is imaginary". Then, you could go crazy with your unicorns and ligers.
 

Several people have said it is meant to be allowed as an option, so the fact that CodeSniffer shouts you down if you use this optional (and valid) syntax means that CodeSniffer PSR2 rules need to change.

Right?

Hopefully my comments above (and maybe below) have explained my reasoning a bit better this time. 

To clarify, all im suggesting here is that the PSR-2 CodeSniffer rules will avoid showing an error for the "no spaces after opening parenthesis" rule in the case that the syntax "if ( ! " is used. 
I assume that technically it's possible?

Yes, this is technical possible, and would be achieved in a similar way to my pseudocode above. It requires an explicit exclusion in the standard itself. Otherwise, I alone am then left defending why someone isn't getting an error for "if ( ! " even though the standard says it should be banned. That's how PHP_CodeSniffer got the check in the first place. Someone reported a bug [1] to me that I was not enforcing this part of the standard. They were right, I simply overlooked it.

It isn't my job to decide what a standard should enforce. I always have to have evidence and reasoning behind each decision and I have to constantly defend standards that I didn't write or even use. The thoughts of a group of people are hard to use as evidence as to why something is allowed. It is much better for that group of people to write their thoughts down, in the standard, as they originally did. Then, I can point to it and link to it and explain to developers why PHP_CodeSniffer is doing what it is doing, just as I did in your original bug report [2].

But you already know this. Your opening comment on this thread makes it clear that you understand that the written PSR2 standard bans"if ( ! " and you are looking for members to tell you that they were wrong and the standard should be ignored in this case. And that is what this discussion is all about. You CAN ignore a written part of the standard, just as developers who use tabs can. But I CANT ignore a written part of the standard because it is the evidence I need to validate why I coded a check the way I did.


Phil Sturgeon

unread,
Apr 7, 2013, 1:21:20 AM4/7/13
to php...@googlegroups.com
Your opening comment on this thread makes it clear that you understand that the written PSR2 standard bans"if ( ! " and you are looking for members to tell you that they were wrong and the standard should be ignored in this case. And that is what this discussion is all about. You CAN ignore a written part of the standard, just as developers who use tabs can. But I CANT ignore a written part of the standard because it is the evidence I need to validate why I coded a check the way I did.

I opened by asking if it was simply overlooked, which many people have said is true. I think we're going to talk in circles on this one, but the facts are:
  1.  "if ( $foo" is banned, because its a space
  2. Nobody thought to add in an exception for whitespaced bangs, even though every readability guide under the sun suggests it
  3. A bunch of people on the core team believe whitespaced bangs are acceptable
  4. Further to that, nobody has ever said "oh yes we intentionally banned them"
  5. If you follow the rule EXPLICITLY then yeah, I get that CodeSniffer should say "ARG" because its a space after the opening bracket
  6. The fact that you dont use PSR-2 suggests why you dont find this as annoying as myself and many others
  7. This does not break compatibility with PSR-2
  8. No its not your job to be psychic, but the intention has since been made clear
  9. You've already got the pseudo code
  10. I'm going to petition like fuck to get a "Unless its a whitespace wrapped bang" into PSR-2
  11. In the meantime adding in this exception would be epic
  12. Unicorns are NOT imaginary. If you wish really hard, one day one will appear outside your window to take you to Disneyland.
I know as a non-PSR-2 user you're probably mostly thinking "well why the heck bother?" but as a PSR-2 user this is insanely frustrating. It's an oversight, it needs clarification and PSR-2 will hopefully eventually get this modification once this bylaw comes into place: https://github.com/padraic/fig-standards/blob/bylaw/amendments/bylaws/004-psr-amendments.md

This would be a non-conflicting change to the standard which means it hopefully wont be too rough, but seeing as clear intent has been made that if ( ! $foo or if (!$foo is optional I would really hope you'd be reasonable enough to push the change through without having to wait an extra month for me to get this rolled into PSR-2.

Phil Sturgeon

unread,
Apr 7, 2013, 1:23:38 AM4/7/13
to php...@googlegroups.com
"just as I did in your original bug report" that's not me bro.

And those two things are referencing different bits of code.

WRONG: "if ( $foo)"
RIGHT: "if ($foo)"
RIGHT: "if (!$foo)"
SHOULD BE ALLOWED: "if ( ! $foo)"

The oversight of the original PSR-2 is they forgot to add an exception for the whitespaced wrapped bangs. That is all.

Greg Sherwood

unread,
Apr 7, 2013, 3:23:07 AM4/7/13
to php...@googlegroups.com

On Sunday, 7 April 2013 15:21:20 UTC+10, Phil Sturgeon wrote:
I know as a non-PSR-2 user you're probably mostly thinking "well why the heck bother?" but as a PSR-2 user this is insanely frustrating. It's an oversight, it needs clarification and PSR-2 will hopefully eventually get this modification once this bylaw comes into place: https://github.com/padraic/fig-standards/blob/bylaw/amendments/bylaws/004-psr-amendments.md

I have absolutely no opinion on if it is worth the time or not. That is absolutely not what I am trying to say. If you think I am having a conversation about if this is a good change for PSR-2 or not, you've misunderstood me.
 
This would be a non-conflicting change to the standard which means it hopefully wont be too rough, but seeing as clear intent has been made that if ( ! $foo or if (!$foo is optional I would really hope you'd be reasonable enough to push the change through without having to wait an extra month for me to get this rolled into PSR-2.

No, sorry. I will not make a change without the PSR being modified. First, nobody actually knows if it is being changed yet. Second, nobody knows if it will be added to PSR-2 or if it will be an entirely new PSR, so I don't even know where to put the code.

All I came here to originally say is:
- the code is banned as per the existing standard (we agree)
- if the voting members think the code should be valid, the PSR should be changed (we agree)
- I don't modify PHP_CodeSniffer standards until the written standard is changed (we don't agree)

But don't worry. As soon as your petition gets the standard amended in some way, I will happily spend my time modifying PHP_CodeSniffer to support it. Even though I don't use the PSR standards, it does not mean that I want to leave them out of date or inaccurate. I want them to be used because I strongly believe in the benefit of coding standards. It doesn't bother me what people use. I just love knowing that people actually do use one.

"just as I did in your original bug report" that's not me bro.

Sorry.
 

André R.

unread,
Apr 15, 2013, 5:14:18 AM4/15/13
to php...@googlegroups.com


I think I agree with Andrew here, this is another personal opinion. Cause to me it looks in-consistent to suddenly allow whit-space in this case and nowhere else.

Paul Jones

unread,
Apr 15, 2013, 9:02:57 AM4/15/13
to php...@googlegroups.com
There's another "RIGHT" option, and that is `if (! $foo)`. No whitespace between the paren and bang; whitespace between the bang and what follows. It's compliant and readable.


-- pmj

Phil Sturgeon

unread,
May 1, 2013, 3:15:11 PM5/1/13
to php...@googlegroups.com
I forgot to mention, I no longer really care about this because I can just do "if (( ! $foo))". I've added it to a wiki page which will act as a "stuff to consider for the next style guide, or amendments, or whatever" list.

Drak

unread,
May 3, 2013, 9:08:32 AM5/3/13
to php...@googlegroups.com
I'm not entirely sure why there is a wiki for this. Surely it's a matter of github ticket and to my mind is misleading.

Drak


On 1 May 2013 20:15, Phil Sturgeon <em...@philsturgeon.co.uk> wrote:
I forgot to mention, I no longer really care about this because I can just do "if (( ! $foo))". I've added it to a wiki page which will act as a "stuff to consider for the next style guide, or amendments, or whatever" list.

--
You received this message because you are subscribed to the Google Groups "PHP Framework Interoperability Group" group.
To unsubscribe from this group and stop receiving emails from it, send an email to php-fig+u...@googlegroups.com.
To post to this group, send email to php...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msg/php-fig/-/QoGtQ-ph5yYJ.

For more options, visit https://groups.google.com/groups/opt_out.
 
 

Phil Sturgeon

unread,
May 4, 2013, 12:49:21 PM5/4/13
to php...@googlegroups.com
Because they are not definitely issues, just things to consider whenever we decide however the hell we're taking care of changes or amendments. I'm not going to make an issue for my whole "Should 'if ( !' be an exception to the NO space after opening parenthesis rules or not" conversation, because it's not definitely an issue. 

Andreas Möller

unread,
May 4, 2013, 1:18:04 PM5/4/13
to php...@googlegroups.com
In my opinion, an exception shouldn't be made.



Attila Szeremi

unread,
Sep 30, 2015, 11:45:06 AM9/30/15
to PHP Framework Interoperability Group
I still like this idea of this, so I'm reviving it.

I would like to add that the exception to `if ( ! ...)` shouldn't just be unconditional.

I would state that e.g. something like this shouldn't be allowed: `if ( ! $this->isSomethingCool() || !$this->isNotCool() || !$this->wellYouKnow())` as you lose what you get in terms of readability.
What you should instead do is `if ( ! ($this->isSomethingCool() && $this->isNotCool() && $this->wellYouKnow()))`, using De Morgan's law

Also, this shouldn't be allowed either: `if ( ! $something || $somethingElse)` as you would lose the readability you'd gain with the spaces around the negation in the first place.

So there should be a restriction that: only a single operator should be after the space exclamation space (a parentheses counts), EXCEPT array and object/class access.

E.g. this should still be okay: `if ( ! $some->obj()->getProperty()['arrayKey'] )`
Reply all
Reply to author
Forward
0 new messages