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

[PSR12] Add return type sniff #2118

Closed
wants to merge 4 commits into from

Conversation

duncan3dc
Copy link
Contributor

Following on from the amazing work done by @akvankorlaar in #1219 this PR:

  • Updates it to handle the breaking change from 1f55125
  • Changes the auto-fixer to prevent it from affecting inline comments (pointed out by @jrfnl and @gsherwood)
  • Add some tests for inline comments
  • Defines it as part of PSR-12

@gsherwood gsherwood added this to the 3.4.0 milestone Sep 5, 2018
@gmponos
Copy link
Contributor

gmponos commented Oct 4, 2018

Hello, I hope this gets merged soon. Or the other PR that you mentioned on the comment.. what ever.

Personally neither on this PR or on the other I see tests related about nullable return types. I hope I didn't missed them. I believe related tests should exist.

@duncan3dc duncan3dc force-pushed the psr-12-return-types branch from c62ae88 to e4f7600 Compare October 4, 2018 19:14
@gsherwood
Copy link
Member

Just a note that the auto-fixer for this does not correctly fix nullable return types:

$ cat temp.php
<?php
function this():  ?string
{
    return null;
}
$ phpcs temp.php --standard=PSR12 --report=diff
--- temp.php
+++ PHP_CodeSniffer
@@ -1,5 +1,5 @@
 <?php
-function this():  ?string
+function this():?string
 {
     return null;
 }

The error messages are also a bit strange as they try to show what the content should be rather than the standard way of saying how much spacing is required. I'll have a look at changing both these things.

@gsherwood gsherwood removed this from the 3.4.0 milestone Nov 6, 2018
@gsherwood
Copy link
Member

I've had a deeper look into this code and I'd basically need to write the sniff to make it work the way I would expect. I appreciate the work people have done on this, but the code style, method of checking spacing, var names, unit tests all need changing if I'm going to accept it into the core and maintain it from now on. I started to make those changes and realised it would be faster to rewrite the sniff and tests, but I don't have the time to do that so I've removed it from the 3.4.0 milestone.

@duncan3dc duncan3dc closed this Nov 6, 2018
@lkraav
Copy link

lkraav commented Feb 18, 2021

Hi @gsherwood since this PR was closed, but no activity on #1219 either since 2018, is there another PR where this is being worked on?

@gsherwood
Copy link
Member

Hi @gsherwood since this PR was closed, but no activity on #1219 either since 2018, is there another PR where this is being worked on?

Not that I know of.

@lkraav
Copy link

lkraav commented Feb 18, 2021

OK, I went with Slevomat for time being so I think that solves it for most cases.

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

Successfully merging this pull request may close these issues.

4 participants