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

Support document formatting #8 #10

Merged
merged 4 commits into from
Sep 6, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions fixtures/format.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
<?php

namespace TestNamespace;




class TestClass
{
public $testProperty;

public function testMethod($testParameter)
{
$testVariable = 123;

if ( empty($testParameter)){
echo 'Empty';
}
}
}
15 changes: 15 additions & 0 deletions fixtures/format_expected.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<?php

namespace TestNamespace;

class TestClass
{
public $testProperty;
public function testMethod($testParameter)
{
$testVariable = 123;
if (empty($testParameter)) {
echo 'Empty';
}
}
}
2 changes: 2 additions & 0 deletions src/LanguageServer.php
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ public function initialize(string $rootPath, int $processId, ClientCapabilities
$serverCapabilities->textDocumentSync = TextDocumentSyncKind::FULL;
// Support "Find all symbols"
$serverCapabilities->documentSymbolProvider = true;
// Support "Format Code"
$serverCapabilities->documentFormattingProvider = true;
return new InitializeResult($serverCapabilities);
}

Expand Down
26 changes: 25 additions & 1 deletion src/Server/TextDocument.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace LanguageServer\Server;

use PhpParser\{Error, Comment, Node, ParserFactory, NodeTraverser, Lexer};
use PhpParser\PrettyPrinter\Standard as PrettyPrinter;
use PhpParser\NodeVisitor\NameResolver;
use LanguageServer\{LanguageClient, ColumnCalculator, SymbolFinder};
use LanguageServer\Protocol\{
Expand All @@ -12,7 +13,9 @@
Diagnostic,
DiagnosticSeverity,
Range,
Position
Position,
FormattingOptions,
TextEdit
};

/**
Expand Down Expand Up @@ -124,4 +127,25 @@ private function updateAst(string $uri, string $content)
$this->asts[$uri] = $stmts;
}
}

/**
* The document formatting request is sent from the server to the client to format a whole document.
*
* @param TextDocumentIdentifier $textDocument The document to format
* @param FormattingOptions $options The format options
* @return TextEdit[]
*/
public function formatting(TextDocumentIdentifier $textDocument, FormattingOptions $options)
{
$nodes = $this->asts[$textDocument->uri];
if (empty($nodes)) {
return [];
}
$prettyPrinter = new PrettyPrinter();
$edit = new TextEdit();
$edit->range = new Range(new Position(0, 0), new Position(PHP_INT_MAX, PHP_INT_MAX));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't found anything about it in protocol spec. Most probably it will depend on client implementation. It works for VS Code but if you feel uncomfortable with it I will reorganize code to store opened documents to calculate end position or traverse to last ast node and get end position from it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's okay as-is. Other options would be to use $nodes[count($nodes) - 1]->getAttribute('fileEndPos') or introduce a new AST object that holds the statements but also the raw file content.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbaeumer is it okay to do this to update the whole document?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felixfbecker yes and no. It is always best if an extension provides a minimal edit set since it will preserve the most markers attached to the text. We have code in place that runs a diff in the whole document gets replaced to compute some more minimal diff. However this might be suboptimal as well. So the more fine grain your edits are the better.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbaeumer Of course a minimal text edit is better, but the default implementation of PHPParser's PrettyPrinter will just spit out the new whole new content. We may customize the behavior later, but for now we just wanted to get the feature done. My question was only about wether it is okay to use a very big number (PHP_INT_MAX) to make it update the whole document (even though the EOF offset is much smaller) or if that is not protocol-conform and causes problems.

$edit->newText = $prettyPrinter->prettyPrintFile($nodes);
return [$edit];
}

}
2 changes: 1 addition & 1 deletion tests/LanguageServerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public function testInitialize()
'workspaceSymbolProvider' => null,
'codeActionProvider' => null,
'codeLensProvider' => null,
'documentFormattingProvider' => null,
'documentFormattingProvider' => true,
'documentRangeFormattingProvider' => null,
'documentOnTypeFormattingProvider' => null,
'renameProvider' => null
Expand Down
32 changes: 31 additions & 1 deletion tests/Server/TextDocumentTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
use PHPUnit\Framework\TestCase;
use LanguageServer\Tests\MockProtocolStream;
use LanguageServer\{Server, Client, LanguageClient};
use LanguageServer\Protocol\{TextDocumentItem, TextDocumentIdentifier, SymbolKind, DiagnosticSeverity};
use LanguageServer\Protocol\{TextDocumentItem, TextDocumentIdentifier, SymbolKind, DiagnosticSeverity, FormattingOptions};
use AdvancedJsonRpc\{Request as RequestBody, Response as ResponseBody};

class TextDocumentTest extends TestCase
Expand Down Expand Up @@ -197,4 +197,34 @@ public function publishDiagnostics(string $uri, array $diagnostics)
]]
], json_decode(json_encode($args), true));
}

public function testFormatting()
{
$textDocument = new Server\TextDocument(new LanguageClient(new MockProtocolStream()));
// Trigger parsing of source
$textDocumentItem = new TextDocumentItem();
$textDocumentItem->uri = 'whatever';
$textDocumentItem->languageId = 'php';
$textDocumentItem->version = 1;
$textDocumentItem->text = file_get_contents(__DIR__ . '/../../fixtures/format.php');
$textDocument->didOpen($textDocumentItem);

// how code should look after formatting
$expected = file_get_contents(__DIR__ . '/../../fixtures/format_expected.php');
// Request formatting
$result = $textDocument->formatting(new TextDocumentIdentifier('whatever'), new FormattingOptions());
$this->assertEquals([0 => [
'range' => [
'start' => [
'line' => 0,
'character' => 0
],
'end' => [
'line' => PHP_INT_MAX,
'character' => PHP_INT_MAX
]
],
'newText' => $expected
]], json_decode(json_encode($result), true));
}
}