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

Fix indentation with OnTypeFormatting method for \n #2922

Merged
merged 14 commits into from
May 30, 2024
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
1 change: 1 addition & 0 deletions extensions/positron-r/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,7 @@
},
"configurationDefaults": {
"[r]": {
"editor.formatOnType": true,
"editor.tabSize": 2,
"editor.wordSeparators": "`~!@#$%^&*()-=+[{]}\\|;:'\",<>/?",
"editor.smartSelect.selectLeadingAndTrailingWhitespace": false
Expand Down
70 changes: 59 additions & 11 deletions extensions/positron-r/src/lsp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {
import { Socket } from 'net';
import { RHelpTopicProvider } from './help';
import { RLspOutputChannelManager } from './lsp-output-channel-manager';

import { R_DOCUMENT_SELECTORS } from './provider';
/**
* The state of the language server.
*/
Expand All @@ -39,6 +39,8 @@ export class ArkLsp implements vscode.Disposable {
private _client?: LanguageClient;

private _state: LspState = LspState.uninitialized;
private _stateEmitter = new vscode.EventEmitter<LspState>();
onDidChangeState = this._stateEmitter.event;

/** Promise that resolves after initialization is complete */
private _initializing?: Promise<void>;
Expand All @@ -51,6 +53,11 @@ export class ArkLsp implements vscode.Disposable {
private readonly _metadata: positron.RuntimeSessionMetadata
) { }

private setState(state: LspState) {
this._state = state;
this._stateEmitter.fire(state);
}

/**
* Activate the language server; returns a promise that resolves when the LSP is
* activated.
Expand Down Expand Up @@ -101,13 +108,7 @@ export class ArkLsp implements vscode.Disposable {
// untitled R files, in-memory R files (e.g. the console), and R / Quarto / R Markdown files on disk.
documentSelector: notebookUri ?
[{ language: 'r', pattern: notebookUri.path }] :
[
{ language: 'r', scheme: 'untitled' },
{ language: 'r', scheme: 'inmemory' }, // Console
{ language: 'r', pattern: '**/*.{r,R}' },
{ language: 'r', pattern: '**/*.{qmd,Qmd}' },
{ language: 'r', pattern: '**/*.{rmd,Rmd}' },
],
R_DOCUMENT_SELECTORS,
synchronize: notebookUri ?
undefined :
{
Expand All @@ -134,7 +135,7 @@ export class ArkLsp implements vscode.Disposable {
// Convert the state to our own enum
switch (event.newState) {
case State.Starting:
this._state = LspState.starting;
this.setState(LspState.starting);
break;
case State.Running:
if (this._initializing) {
Expand All @@ -146,14 +147,14 @@ export class ArkLsp implements vscode.Disposable {
}
out.resolve();
}
this._state = LspState.running;
this.setState(LspState.running);
break;
case State.Stopped:
if (this._initializing) {
LOGGER.info(`ARK (R ${this._version}) language client init failed`);
out.reject('Ark LSP client stopped before initialization');
}
this._state = LspState.stopped;
this.setState(LspState.stopped);
break;
}
LOGGER.info(`ARK (R ${this._version}) language client state changed ${oldState} => ${this._state}`);
Expand Down Expand Up @@ -215,6 +216,53 @@ export class ArkLsp implements vscode.Disposable {
return this._state;
}

/**
* Wait for the LSP to be connected.
*
* Resolves to `true` once the LSP is connected. Resolves to `false` if the
* LSP has been stopped. Rejects if the LSP fails to start.
*/
async wait(): Promise<boolean> {
switch (this.state) {
case LspState.running: return true;
case LspState.stopped: return false;

case LspState.starting: {
// Inherit init promise. This can reject if init fails.
await this._initializing;
return true;
}

case LspState.uninitialized: {
const handles = new PromiseHandles<boolean>();

const cleanup = this.onDidChangeState(state => {
let out: boolean;
switch (this.state) {
case LspState.running: out = true; break;
case LspState.stopped: out = false; break;
case LspState.uninitialized: return;
case LspState.starting: {
// Inherit init promise
if (this._initializing) {
cleanup.dispose();
this._initializing.
then(() => handles.resolve(true)).
catch((err) => handles.reject(err));
}
return;
}
}

cleanup.dispose();
handles.resolve(out);
});

return await handles.promise;
}
}
}

/**
* Registers additional Positron-specific LSP methods. These programmatic
* language features are not part of the LSP specification, and are
Expand Down
10 changes: 10 additions & 0 deletions extensions/positron-r/src/provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,16 @@ import { LOGGER } from './extension';
import { readLines } from './util';
import { EXTENSION_ROOT_DIR, MINIMUM_R_VERSION } from './constants';

// We don't give this a type so it's compatible with both the VS Code
// and the LSP types
export const R_DOCUMENT_SELECTORS = [
{ language: 'r', scheme: 'untitled' },
{ language: 'r', scheme: 'inmemory' }, // Console
{ language: 'r', pattern: '**/*.{r,R}' },
{ language: 'r', pattern: '**/*.{qmd,Qmd}' },
{ language: 'r', pattern: '**/*.{rmd,Rmd}' },
];

/**
* Discovers R language runtimes for Positron; implements
* positron.LanguageRuntimeDiscoverer.
Expand Down
10 changes: 10 additions & 0 deletions extensions/positron-r/src/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -531,6 +531,16 @@ export class RSession implements positron.LanguageRuntimeSession, vscode.Disposa
this._lsp.activate(port, this.context);
}

/**
* Wait for the LSP to be connected.
*
* Resolves to `true` once the LSP is connected. Resolves to `false` if the
* LSP has been stopped. Rejects if the LSP fails to start.
*/
async waitLsp(): Promise<boolean> {
return await this._lsp.wait();
}

private async startDap(): Promise<void> {
if (this._kernel) {
const port = await this.adapterApi!.findAvailablePort([], 25);
Expand Down
37 changes: 34 additions & 3 deletions extensions/positron-r/src/test/indentation.test.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import * as vscode from 'vscode';
import * as positron from 'positron';
import * as assert from 'assert';
import * as fs from 'fs';
import { CURSOR, type, withFileEditor } from './editor-utils';
import { EXTENSION_ROOT_DIR } from '../constants';
import { removeLeadingLines } from '../util';
import { delay, removeLeadingLines } from '../util';
import { RSession } from '../session';

const snapshotsFolder = `${EXTENSION_ROOT_DIR}/src/test/snapshots`;
const snippetsPath = `${snapshotsFolder}/indentation-cases.R`;
Expand All @@ -30,6 +32,22 @@ suite('Indentation', () => {
// it if that's a bug to fix.
test('Regenerate and check', async () => {
await init();

// There doesn't seem to be a method that resolves when a language is
// both discovered and ready to be started
let info;
while (true) {
try {
info = await positron.runtime.getPreferredRuntime('r');
break;
} catch (_) {
await delay(50);
}
}

let ses = await positron.runtime.startLanguageRuntime(info!.runtimeId, 'Snapshot tests') as RSession;
await ses.waitLsp();

const expected = fs.readFileSync(snapshotsPath, 'utf8');
const current = await regenerateIndentSnapshots();

Expand All @@ -54,9 +72,22 @@ async function regenerateIndentSnapshots() {
for (const snippet of snippets) {
const bareSnippet = snippet.split('\n').slice(0, -1).join('\n');

await withFileEditor(snippet, 'R', async (_editor, doc) => {
await withFileEditor(snippet, 'R', async (editor, doc) => {
// Type one newline character to trigger indentation
await type(doc, `\n${CURSOR}`);
await type(doc, '\n');

await vscode.commands.executeCommand('vscode.executeFormatOnTypeProvider',
doc.uri,
editor.selection.start,
'\n',
{
insertSpaces: true,
tabSize: 4,
}
);

await type(doc, `${CURSOR}`);

const snapshot = removeLeadingLines(doc.getText(), /^$|^#/);
snapshots.push(bareSnippet + '\n# ->\n' + snapshot);
});
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
{
"workbench.localHistory.enabled": false,
"editor.insertSpaces": true,
"editor.indentSize": 4,
"editor.tabSize": 4,
"[r]" : {
"editor.insertSpaces": true,
"editor.indentSize": 4,
"editor.tabSize": 4
}
}
44 changes: 36 additions & 8 deletions extensions/positron-r/src/test/snapshots/indentation-cases.R
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,11 @@ data |>"<>"

# ---
# Starting a pipeline (one empty line)
# FIXME
data |>
"<>"

# ---
# Starting a pipeline (multiple empty lines)
# FIXME
data |>

"<>"
Expand Down Expand Up @@ -53,14 +51,12 @@ data |>

# ---
# Continuing a one-liner pipeline (comment line)
# FIXME
data |>
fn1() |>
# foo"<>"

# ---
# Continuing a one-liner pipeline (after a comment line)
# FIXME
data |>
fn1() |>
# foo
Expand All @@ -75,7 +71,6 @@ data |>

# ---
# Continuing a multi-liner pipeline
# FIXME
data |>
fn1(
x,
Expand All @@ -84,7 +79,6 @@ data |>

# ---
# Continuing a multi-liner pipeline (trailing expression)
# FIXME
data |>
fn1(
x,
Expand All @@ -103,20 +97,55 @@ data |>

# ---
# Dedent after pipeline (multiple lines)
# FIXME
# https://github.com/posit-dev/positron/issues/2764
data |>
fn1() |>
fn2(
"arg"
)"<>"

# ---
# Dedent after pipeline (token)
1 +
foo(
x
) +
bar"<>"

# ---
# Dedent after pipeline (nested)
{
1 +
foo(
x
) +
bar"<>"
}

# ---
# Stickiness of dedent after pipeline
# https://github.com/posit-dev/positron/issues/1727
data |>
fn()
"<>"

# ---
# Stickiness of dedent after pipeline (nested)
{
data |>
fn()
"<>"
}

# ---
# Stickiness of dedent after pipeline (nested)
{
fn() %>%

foo"<>"
}


# ---
# Stickiness of dedent after pipeline (trailing comment)
data |>
Expand Down Expand Up @@ -146,7 +175,6 @@ for (i in NA) NULL"<>"
# ---
# Indent after finished loop (call)
# https://github.com/posit-dev/positron/issues/1880
# FIXME
for (i in 1) fn()"<>"

# ---
Expand Down
Loading
Loading