Use PHP_CodeSniffer as a formatter #35

mniewrzal · 2016-09-27T09:26:05Z

No description provided.

codecov-io · 2016-09-27T09:32:17Z

Current coverage is 89.91% (diff: 91.48%)

Merging #35 into master will increase coverage by 0.22%

@@             master        #35   diff @@
==========================================
  Files            21         23     +2   
  Lines           456        496    +40   
  Methods          66         70     +4   
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            409        446    +37   
- Misses           47         50     +3   
  Partials          0          0

Diff Coverage	File Path
•••••••• 86%	new src/Formatter.php
•••••••••• 100%	src/Protocol/TextEdit.php
•••••••••• 100%	src/utils.php
•••••••••• 100%	src/PhpDocument.php

Powered by Codecov. Last update 18ac760...47482fc

felixfbecker

LGTM overall. Just some minor changes and more test coverage needed (see codecov)

felixfbecker · 2016-09-28T10:20:50Z

src/Server/Formatter.php

+
+use LanguageServer\Protocol\TextEdit;
+use LanguageServer\Protocol\Range;
+use LanguageServer\Protocol\Position;


Please use a group use statement here

felixfbecker · 2016-09-28T10:24:12Z

src/Server/Formatter.php

+    public function format(string $content, string $uri)
+    {
+        // remove 'file://' prefix
+        $uri = substr($uri, 7);


I don't think this is the right way to do it - an input of file:///c:/users/felix/test would result in /c:/users/felix/test which is not a valid path. #31 adds a path2uri function, I would propose to add the reverse uri2path function (with some tests) https://github.com/MadHed/php-language-server/blob/7b0b9b890cabd3f0c2e1d7f10314ed61f53df110/src/utils.php#L23-L33

For now I added new method to Formatter to avoid conflicts with #31. Later I can move it to utils.php

felixfbecker · 2016-09-28T10:25:07Z

src/Server/Formatter.php

+            $currentDir = dirname($currentDir);
+        } while ($currentDir !== '.' && $currentDir !== $lastDir);
+
+        $standard = \PHP_CodeSniffer::getConfigData('default_standard');


$standard = \PHP_CodeSniffer::getConfigData('default_standard') ?? 'PSR2'

kaloyan-raev · 2016-09-29T20:09:42Z

I noticed that a big mess happens when formatting (in VSCode) the SymbolFinder.php file - in this part: https://github.com/felixfbecker/php-language-server/blob/master/src/SymbolFinder.php#L54-L63

mniewrzal · 2016-09-29T21:36:52Z

Code Sniffer is failing to format SymbolFinder.php. I added simple error handling. Tomorrow I will report this problem to PHPCS.

felixfbecker · 2016-09-30T07:29:47Z

src/Server/Formatter.php

-        $file->fixer->fixFile();
+        $fixed = $file->fixer->fixFile();
+        if (!$fixed) {
+            throw new ResponseError("Unable to format file", ErrorCode::INTERNAL_ERROR);


Please use single quotes

mniewrzal · 2016-09-30T08:54:23Z

I reported formatting issue: squizlabs/PHP_CodeSniffer#1180

felixfbecker · 2016-09-30T09:13:17Z

Will merge #31 first, will probably be some refactoring for you to rebase

mniewrzal · 2016-09-30T09:17:52Z

Ok, I'm aware of that ;)

felixfbecker · 2016-09-30T09:39:06Z

Just merged #31

felixfbecker · 2016-10-04T07:21:09Z

src/Protocol/TextEdit.php

@@ -22,4 +22,10 @@ class TextEdit
     * @var string
     */
    public $newText;
+
+    public function __construct(Range $range = null, string $newText)


Non-optional parameter cannot follow an optional param. Both should be optional to allow constructing a instance and then setting the properties manually

felixfbecker · 2016-10-04T07:26:40Z

tests/Utils/FileUriTest.php

+        $uri = "/home/foo/bar/Test.php";
+        $this->assertEquals("/home/foo/bar/Test.php", \LanguageServer\uriToPath($uri));
+
+        $uri = "/home/foo space/bar/Test.php";


Not a valid URI, space needs to be %20

felixfbecker · 2016-10-04T07:27:22Z

tests/Utils/FileUriTest.php

+        $uri = "file:///c:/home/foo/bar/Test.php";
+        $this->assertEquals("c:\\home\\foo\\bar\\Test.php", \LanguageServer\uriToPath($uri));
+
+        $uri = "c:\\home\\foo\\bar\\Test.php";


That's not a valid URI so I don't think we need to test against it

felixfbecker · 2016-10-04T07:28:00Z

src/utils.php

+        return $uri;
+    }
+    $path = substr($uri, $position + 3);
+    return strpos($path, ':') ? str_replace('/', "\\", substr($path, 1)) : $path;


Not that happy with this implementation.

If possible I would like to use parse_url

Needs to handle %-encoding

Needs to replace / with DIRECTORY_SEPERATOR

felixfbecker · 2016-10-04T07:29:03Z

tests/Server/TextDocumentTest.php

+        $textDocument = new Server\TextDocument($project, $client);
+
+        $result = $textDocument->formatting(new TextDocumentIdentifier('whatever'), new FormattingOptions());
+        $this->assertEquals([], json_decode(json_encode($result), true));


Imo an invalid URI should throw an error instead

Actual implementation will create PhpDocument for invalid/non-existing uri (Project::getDocument). I can try to reorganize this with this change but I think it should be done as separate issue.

Fair enough.

felixfbecker · 2016-10-04T07:30:22Z

src/Formatter.php

+        do {
+            $default = $currentDir . DIRECTORY_SEPARATOR . 'phpcs.xml';
+            if (is_file($default) === true) {
+                return array($default);


Use short array syntax

felixfbecker

Tests for uriToPath and pathToUri should be symmetric, could you clean them up a bit?

felixfbecker · 2016-10-04T10:56:34Z

src/utils.php

-    $path = substr($uri, $position + 3);
-    return strpos($path, ':') ? str_replace('/', "\\", substr($path, 1)) : $path;
+    $path = urldecode(parse_url($uri)['path']);
+    return strpos($path, ':') ? str_replace(\DIRECTORY_SEPARATOR, '\\', substr($path, 1)) : $path;


strpos may return 0 or false, should check explicitly for false here

It's str_replace($search, $replace, $subject), we need to search for '/' and replace it with DIRECTORY_SEPERATOR

Should be ok right now.

felixfbecker · 2016-10-04T10:58:16Z

tests/Utils/FileUriTest.php

+        $uri = 'file:///a/b/c/test.txt';
+        $this->assertEquals('/a/b/c/test.txt', \LanguageServer\uriToPath($uri));
+
+        $uri = 'file:///c%3A/foo/bar.baz';


The colon after the drive letter is typically not encoded afaik
https://en.wikipedia.org/wiki/File_URI_scheme#Windows_2

Changed to simple colon.

kaloyan-raev · 2016-10-05T14:57:38Z

src/Formatter.php

+        if ($content === $new) {
+            return [];
+        }
+        return [new TextEdit(new Range(new Position(0, 0), new Position(PHP_INT_MAX, PHP_INT_MAX)), $new)];


Eclipse Che requires an accurate calculation of the end position. Although VSCode can handle this well, the editor in Che just breaks if any negative values or big values beyond the document end are provided.

Ok, I will try to calculate end position.

Thanks! This now works very well in Che.

kaloyan-raev · 2016-10-10T06:08:26Z

@felixfbecker is there anything else pending (except resolving conflicts) for this PR to be merged?

felixfbecker

There a few minor nitpicks which I will just fix myself if you don't beat me to it. The important stuff really is support for non-file URIs

felixfbecker · 2016-10-10T07:16:12Z

src/Formatter.php

+    {
+        $path = uriToPath($uri);
+        $cs = new \PHP_CodeSniffer();
+        $cs->initStandard($this->findConfiguration($path));


Will this also work in environments where

URIs are not file URIs

there is no config file in the project
?

Will this also work in environments where

URIs are not file URIs

For directory URI it wont work if URI = directory with config file but in this case it will load default configuration. Should we be worried about such cases? Do you notices URIs different than file?

there is no config file in the project ?

If config file is not found PSR2 standard is loaded.

What I meant was that we need to handle URIs that don't use the file:// scheme, but something like vfs://.

Ok, understand now. We can think about it later.

felixfbecker · 2016-10-10T07:20:27Z

src/utils.php

+ */
+function uriToPath(string $uri)
+{
+    $filepath = urldecode(parse_url($uri)['path']);


A URI can have any protocol, but only file URIs can be mapped to a path. We need to throw an InvalidArgumentException if the URI is not a file:// URI and also handle/test that case in the implementation.

felixfbecker · 2016-10-10T07:23:37Z

src/utils.php

+function uriToPath(string $uri)
+{
+    $filepath = urldecode(parse_url($uri)['path']);
+    return strpos($filepath, ':') === false ? $filepath : str_replace('/', '\\', $filepath);


This is a little brittle. There are also implementations that use | instead of the colon on windows. You don't need to test against that, but it would be better to simply do str_replace('/', DIRECTORY_SEPERATOR, $filepath)

https://en.wikipedia.org/wiki/File_URI_scheme#Windows

Forget what I said. the directory separator should of course be determined by the URI, not the runtime OS.

felixfbecker · 2016-10-10T07:26:01Z

src/Formatter.php

+    private function calculateEndPosition(string $content): Position
+    {
+        $lines = explode("\n", $content);
+        return new Position(sizeof($lines) - 1, strlen(end($lines)));


Please use count, sizeof is just an alias and PHP has so many aliases nobody can remember them all 😄

felixfbecker · 2016-10-10T07:26:58Z

src/Formatter.php

+     * @param string $uri            
+     * @return string[]
+     */
+    private function findConfiguration(string $uri)


The parameter is named $uri, but you call it with a path, not a URI. Some description for the function and params would also be nice

felixfbecker · 2016-10-10T07:30:56Z

tests/FormatterTest.php

+        $this->assertTrue($edits == []);
+    }
+
+}


Missing newline - I don't know which editor you use but I recommend you to look for an EditorConfig extension 😉

felixfbecker · 2016-10-10T07:33:27Z

src/Formatter.php

+use LanguageServer\Protocol\{TextEdit, Range, Position, ErrorCode};
+use AdvancedJsonRpc\ResponseError;
+
+class Formatter


Initially I was wondering why this needs to be a class that is instantiated when it has no properties, constructor or internal state. Everything could just be pure functions. But it's probably better to encapsulate the private functions. One possibility would be though to make them static and the class abstract. Opinions on this?

I added this class to prevent messing with PhpDocument in future. Abstract class and static methods sounds ok for me.

felixfbecker · 2016-10-10T07:34:09Z

tests/Server/TextDocumentTest.php

+        $textDocument = new Server\TextDocument($project, $client);
+
+        $result = $textDocument->formatting(new TextDocumentIdentifier('whatever'), new FormattingOptions());
+        $this->assertEquals([], json_decode(json_encode($result), true));


Fair enough.

felixfbecker · 2016-10-10T07:34:28Z

tests/Server/TextDocumentTest.php

+        $textDocument = new Server\TextDocument($project, $client);
+
+        $result = $textDocument->formatting(new TextDocumentIdentifier('whatever'), new FormattingOptions());
+        $this->assertEquals([], json_decode(json_encode($result), true));


Don't need to json_decode(json_encode())

felixfbecker · 2016-10-10T07:36:38Z

src/Formatter.php

+    public function format(string $content, string $uri)
+    {
+        $path = uriToPath($uri);
+        $cs = new \PHP_CodeSniffer();


Please add a use PHP_CodeSniffer at the top. This declares the dependency on the class and avoids having to use absolute references

felixfbecker · 2016-10-10T09:18:22Z

Fixed the style issues

felixfbecker · 2016-10-10T11:47:51Z

I think this is good to go, just needs a rebase

felixfbecker mentioned this pull request Sep 28, 2016

Format Code commad adds '\' before a class name #36

Closed

felixfbecker requested changes Sep 28, 2016

View reviewed changes

mniewrzal mentioned this pull request Sep 28, 2016

Add configuration for PHP formatting options #29

Closed

felixfbecker requested changes Sep 30, 2016

View reviewed changes

mniewrzal force-pushed the PHP_CodeSniffer branch from fb8848f to 0ba3304 Compare September 30, 2016 11:16

felixfbecker requested changes Oct 4, 2016

View reviewed changes

kaloyan-raev suggested changes Oct 5, 2016

View reviewed changes

kaloyan-raev approved these changes Oct 5, 2016

View reviewed changes

felixfbecker mentioned this pull request Oct 8, 2016

Format code option does nothing felixfbecker/vscode-php-intellisense#19

Closed

sunverwerth mentioned this pull request Oct 8, 2016

Add linting #12

Closed

felixfbecker requested changes Oct 10, 2016

View reviewed changes

felixfbecker approved these changes Oct 10, 2016

View reviewed changes

felixfbecker mentioned this pull request Oct 10, 2016

Only hold content for open files in memory #60

Closed

Michal Niewrzal and others added 6 commits October 10, 2016 14:45

Use PHP_CodeSniffer as a formatter

6b03196

Small cleanup

552d645

Use PHP_CodeSniffer as a formatter

8de1884

Use PHP_CodeSniffer as a formatter

8268d15

Added end position calculation

4c160e1

Minor code style fixes

f1c7991

felixfbecker and others added 3 commits October 10, 2016 14:48

More code style fixes

a43d2b2

Throw exception on invalid file URI

6611a30

Changes after review

47482fc

felixfbecker force-pushed the PHP_CodeSniffer branch from 1642754 to 47482fc Compare October 10, 2016 12:56

felixfbecker merged commit e75c159 into felixfbecker:master Oct 10, 2016

mniewrzal deleted the PHP_CodeSniffer branch October 10, 2016 13:22

Use PHP_CodeSniffer as a formatter #35

Use PHP_CodeSniffer as a formatter #35

Conversation

mniewrzal commented Sep 27, 2016

codecov-io commented Sep 27, 2016 • edited Loading

Current coverage is 89.91% (diff: 91.48%)

felixfbecker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixfbecker Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaloyan-raev commented Sep 29, 2016 • edited Loading

mniewrzal commented Sep 29, 2016

Choose a reason for hiding this comment

mniewrzal commented Sep 30, 2016

felixfbecker commented Sep 30, 2016 • edited Loading

mniewrzal commented Sep 30, 2016

felixfbecker commented Sep 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixfbecker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaloyan-raev commented Oct 10, 2016

felixfbecker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixfbecker Oct 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixfbecker commented Oct 10, 2016

felixfbecker commented Oct 10, 2016

codecov-io commented Sep 27, 2016 •

edited

Loading

felixfbecker Sep 28, 2016 •

edited

Loading

kaloyan-raev commented Sep 29, 2016 •

edited

Loading

felixfbecker commented Sep 30, 2016 •

edited

Loading

felixfbecker Oct 10, 2016 •

edited

Loading