Skip to content

More smart PrettyPrinter formatting#24

Merged
omochi merged 4 commits intoomochi:mainfrom
sidepelican:short_output
Nov 2, 2022
Merged

More smart PrettyPrinter formatting#24
omochi merged 4 commits intoomochi:mainfrom
sidepelican:short_output

Conversation

@sidepelican
Copy link
Collaborator

sidepelican/CallableKit#4 において言及した、生成コードフォーマットを改善します。
主に小さな1つの変更と、大きな1つの変更があります

importDeclの分割

importする識別子の数に応じて改行の有無を変化させます

  • 変更前
import { 
    A,
    B
} from "./ab.js";

import { 
    C
} from "./c.js";

import {
    D,
    E,
    F,
    G,
    H,
    I
} from "./defghi.js";
  • 変更後
import { A, B } from "./ab.js";

import { C } from "./c.js";

import {
    D,
    E,
    F,
    G,
    H,
    I
} from "./defghi.js";

PrettyPrinterにスコープの概念の導入

PrettyPrinterに現在どのスコープで描画されているかのパラメータを与えることで、スコープに応じて上下のdeclとの間に改行を入れるかどうかを判断できるようにしました。
これにより自身とその上のdeclが同一種類のdeclだった場合に、改行を入れるべきかどうか判断できるようになりました。

  • 変更前
import { A, B } from "./ab.js";

import { C } from "./c.js";

export interface I {
    value1: A;

    value2: B;

    f1();

    f2();
}

export function foo() {
    const c1: C | undefined = undefined;

    const c2: C | undefined = undefined;
}

export const c1: C | undefined = undefined;

export const c2: C | undefined = undefined;
  • 変更後
import { A, B } from "./ab.js";
import { C } from "./c.js";

export interface I {
    value1: A;
    value2: B;

    f1();
    f2();
}

export function foo() {
    const c1: C | undefined = undefined;
    const c2: C | undefined = undefined;
}

export const c1: C | undefined = undefined;

export const c2: C | undefined = undefined;

@omochi
Copy link
Owner

omochi commented Nov 2, 2022

TSImportDecl の一行モード

これは他と同じ方針になっていて良いと思います。
引数等は4つから行区切りなんですが、
importは7つぐらい並べても良さそうかなとは思います。

PrettyPrinterの改修による改行表現の改善

目的と追加したい機能については賛成です。
しかし、実装上の設計方針で微妙なところがあります。

PrettyPrinter は、TypeScript ASTには関心がない、
汎用的なテキスト処理のヘルパーとして設計しています。
インデントの機能が追加されたシンプルなテキストの出力の補助コンポーネントです。

そのインデント機能を利用しつつ、
適切なフォーマットにテキストを整形する責任を持つのは、
それのユーザー側の仕事と考えています。

こうすることで PrettyPrinter を再利用性の高いものにする事と、
フォーマットに関するロジックをAST側に集約する事で見通しを良くする狙いがあります。

そのためこの変更内容だと、 PrettyPrinter が自身の機能には関係がない、
BlockScope を保持するだけの機能を、
TypeScript ASTの都合で導入している点が良くないです。

しかし、しばらく考えてみたんですが、
同等のロジックを現時点のコードベースで PrettyPrinter に影響を与えない形で
実装するのは難しいです。
特に、 function scope 属性をネストした TSBlockStmt の子孫まで伝搬させるのが困難です。

僕の考える正しいアプローチは
DependencyScanner のような形で、
新たに Formatter オブジェクトを作成して、
それが内部で PrettyPrinter を補助的に利用する、
という設計だと思いますが、
これはまあまあ大きな変更が必要です。

上述の importDecl の基準値についても、
smallCountPrettyPrinter に付いてるのは同じような関心の侵食があるので、
それも Formatter に移し替えるべきでしょうね。

結論としては、いったん機能実現を優先してマージするけど、
将来的には設計を見直したいです。

@omochi omochi merged commit 8428584 into omochi:main Nov 2, 2022
@omochi omochi mentioned this pull request Nov 2, 2022
@sidepelican
Copy link
Collaborator Author

BlockScope を保持するだけの機能を、
TypeScript ASTの都合で導入している

そうですね。この懸念はありましたが、同じく機能を実現するために良い方法が他に思いつかず、smallCount という似たようなものの存在から割れ窓で組み込んだ形ではあります。

他には @ThreadLocal を作ってそこに保持するとかも考えましたが、コンテキストオブジェクトの存在が非自明なのもうーんと思ったのでやめました。

PrettyPrintable の上位概念ができたり、 PrettyPrinter がジェネリックにコンテキストオブジェクトを保持する設計とかそういう方針は良いかなと思います

@sidepelican sidepelican deleted the short_output branch December 16, 2022 05:38
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.

2 participants