mirror of
https://github.com/dainakai/obsidian-yaml-table.git
synced 2026-07-22 05:48:44 +00:00
Fix: Address review comments
This commit is contained in:
dainakai
parent
9755197475
commit
b763f843cb
3 changed files with 84 additions and 32 deletions
2
LICENSE
2
LICENSE
|
|
@ -1,6 +1,6 @@
|
|||
MIT License
|
||||
|
||||
Copyright (c) 2023 dainakai
|
||||
Copyright (c) 2025 dainakai
|
||||
|
||||
Permission is hereby granted, free of charge, to any person obtaining a copy
|
||||
of this software and associated documentation files (the "Software"), to deal
|
||||
|
|
|
|||
52
docs/review_summary.md
Normal file
52
docs/review_summary.md
Normal file
|
|
@ -0,0 +1,52 @@
|
|||
# Obsidianプラグインレビュー指摘事項まとめ
|
||||
|
||||
## 概要
|
||||
|
||||
Obsidianプラグイン `obsidian-yaml-table` のプルリクエストレビューで指摘された事項とその修正案を以下にまとめます。
|
||||
|
||||
## 指摘事項と修正案
|
||||
|
||||
### 1. Copyrightの年号
|
||||
|
||||
* **指摘内容**: `LICENSE` ファイルに記載されているCopyrightの年号が古い (2023年)。
|
||||
* 参照: [LICENSE#L3](https://github.com/dainakai/obsidian-yaml-table/blob/97551974752d91fc5c8fed8f112d86ca4b612157/LICENSE#L3)
|
||||
* **修正案**: Copyrightの年号を現在の2025年に更新します。
|
||||
|
||||
### 2. `js-yaml` の直接利用
|
||||
|
||||
* **指摘内容**: YAMLのパースに `js-yaml` ライブラリを直接インポートして使用している。
|
||||
* 参照:
|
||||
* `import * as yaml from \'js-yaml\';` ([src/main.ts#L11](https://github.com/dainakai/obsidian-yaml-table/blob/97551974752d91fc5c8fed8f112d86ca4b612157/src/main.ts#L11))
|
||||
* `const data = yaml.load(source);` ([src/main.ts#L66](https://github.com/dainakai/obsidian-yaml-table/blob/97551974752d91fc5c8fed8f112d86ca4b612157/src/main.ts#L66))
|
||||
* **修正案**: Obsidian APIが提供する `parseYaml` および `stringifyYaml` 関数を利用するようにコードを修正します。これにより、Obsidian本体との互換性やパフォーマンスが向上する可能性があります。
|
||||
* **具体的な修正**:
|
||||
1. `src/main.ts` の先頭で `import * as yaml from \'js-yaml\';` を削除します。
|
||||
2. 代わりに `import { parseYaml } from \'obsidian\';` を追加します (もしYAMLを文字列化する機能も使っていれば `stringifyYaml` もインポート)。
|
||||
3. `yaml.load(source)` となっている箇所を `parseYaml(source)` に置き換えます。
|
||||
|
||||
### 3. 非推奨の `MarkdownRenderer.renderMarkdown` メソッドの使用
|
||||
|
||||
* **指摘内容**: 非推奨 (deprecated) となった `MarkdownRenderer.renderMarkdown` メソッドが3箇所で使用されている。
|
||||
* 参照:
|
||||
* [src/main.ts#L174](https://github.com/dainakai/obsidian-yaml-table/blob/97551974752d91fc5c8fed8f112d86ca4b612157/src/main.ts#L174)
|
||||
* [src/main.ts#L224](https://github.com/dainakai/obsidian-yaml-table/blob/97551974752d91fc5c8fed8f112d86ca4b612157/src/main.ts#L224)
|
||||
* [src/main.ts#L265](https://github.com/dainakai/obsidian-yaml-table/blob/97551974752d91fc5c8fed8f112d86ca4b612157/src/main.ts#L265)
|
||||
* **修正案**: ObsidianのドキュメントやAPIリファレンスを確認し、推奨されている代替のMarkdownレンダリング用メソッド `MarkdownRenderer.render(markdown: string, el: HTMLElement, sourcePath: string, component: Component): Promise<void>` に置き換えます。
|
||||
* **具体的な修正**:
|
||||
1. `MarkdownRenderer.renderMarkdown(markdown, el, sourcePath, component)` の呼び出しを `await MarkdownRenderer.render(markdown, el, sourcePath, component)` に変更します。
|
||||
2. このメソッドは `Promise` を返すため、呼び出し元の関数が `async` で宣言されていることを確認し、必要であれば `async` を追加します。
|
||||
3. `component` 引数は `null` を許容しないため、`null` を渡している場合は、適切な `Component` のインスタンス (通常はプラグインのインスタンス `this`) を渡すようにします。
|
||||
|
||||
### 4. 設定タブのトップレベル見出し
|
||||
|
||||
* **指摘内容**: プラグインの設定タブにトップレベルの見出し (`h2` 要素で "YAML Table Settings") を追加している。これは、複数の設定セクションがない場合には避けるべきとされています。
|
||||
* 参照:
|
||||
* `containerEl.createEl(\'h2\', {text: \'YAML Table Settings\'});` ([src/main.ts#L282](https://github.com/dainakai/obsidian-yaml-table/blob/97551974752d91fc5c8fed8f112d86ca4b612157/src/main.ts#L282))
|
||||
* [Obsidian Plugin Guidelines](https://docs.obsidian.md/Plugins/Releasing/Plugin+guidelines#Only+use+headings+under+settings+if+you+have+more+than+one+section)
|
||||
* **修正案**: プラグイン設定に複数のセクションが存在しない場合は、このトップレベルの見出し (`h2` 要素) を削除します。
|
||||
* **具体的な修正**: `src/main.ts` の L282 にある `containerEl.createEl(\'h2\', {text: \'YAML Table Settings\'});` の行を削除します。
|
||||
|
||||
## 今後の対応
|
||||
|
||||
上記の指摘事項に基づき、コードの修正を行います。
|
||||
特に `MarkdownRenderer.renderMarkdown` の代替メソッドについては、Obsidianの公式ドキュメントを参照して適切な対応を行います。
|
||||
62
src/main.ts
62
src/main.ts
|
|
@ -6,9 +6,9 @@ import {
|
|||
MarkdownPostProcessorContext,
|
||||
MarkdownView,
|
||||
MarkdownRenderer,
|
||||
Component
|
||||
Component,
|
||||
parseYaml
|
||||
} from 'obsidian';
|
||||
import * as yaml from 'js-yaml';
|
||||
|
||||
interface YAMLTableSettings {
|
||||
language: string;
|
||||
|
|
@ -38,7 +38,7 @@ export default class YAMLTablePlugin extends Plugin {
|
|||
}
|
||||
|
||||
// Processor to convert YAML to HTML table or list
|
||||
yamlTableProcessor(source: string, el: HTMLElement, ctx: MarkdownPostProcessorContext) {
|
||||
async yamlTableProcessor(source: string, el: HTMLElement, ctx: MarkdownPostProcessorContext) {
|
||||
let captionText: string | null = null;
|
||||
const view = this.app.workspace.getActiveViewOfType(MarkdownView);
|
||||
if (view) {
|
||||
|
|
@ -63,16 +63,16 @@ export default class YAMLTablePlugin extends Plugin {
|
|||
el.appendChild(captionEl);
|
||||
}
|
||||
|
||||
const data = yaml.load(source);
|
||||
const data = parseYaml(source);
|
||||
|
||||
let renderedElement: HTMLElement | null = null;
|
||||
|
||||
if (Array.isArray(data)) {
|
||||
// Handle top-level array data
|
||||
renderedElement = this.createHTMLElementForArray(data, ctx.sourcePath, this);
|
||||
renderedElement = await this.createHTMLElementForArray(data, ctx.sourcePath, this);
|
||||
} else if (data !== null && typeof data === 'object') {
|
||||
// Handle top-level object data
|
||||
renderedElement = this.createTableFromObject(data as Record<string, unknown>, ctx.sourcePath, this);
|
||||
renderedElement = await this.createTableFromObject(data as Record<string, unknown>, ctx.sourcePath, this);
|
||||
}
|
||||
|
||||
// Add .yaml-table class if the rendered element is a TABLE
|
||||
|
|
@ -119,7 +119,7 @@ export default class YAMLTablePlugin extends Plugin {
|
|||
}
|
||||
|
||||
// Creates a TABLE element specifically for JS objects (key-value pairs)
|
||||
createTableFromObject(data: Record<string, unknown>, sourcePath: string, component: Component): HTMLTableElement | null {
|
||||
async createTableFromObject(data: Record<string, unknown>, sourcePath: string, component: Component): Promise<HTMLTableElement | null> {
|
||||
if (Object.keys(data).length === 0) {
|
||||
return null; // Return null for empty objects
|
||||
}
|
||||
|
|
@ -133,19 +133,19 @@ export default class YAMLTablePlugin extends Plugin {
|
|||
|
||||
const keyCell = document.createElement('th');
|
||||
keyCell.setAttribute('scope', 'row');
|
||||
MarkdownRenderer.renderMarkdown(String(key), keyCell, sourcePath, component);
|
||||
await MarkdownRenderer.render(this.app, String(key), keyCell, sourcePath, component);
|
||||
row.appendChild(keyCell);
|
||||
|
||||
const valueCell = row.insertCell();
|
||||
// Explicitly pass the object's value to renderValue
|
||||
this.renderValue(data[key], valueCell, sourcePath, component);
|
||||
await this.renderValue(data[key], valueCell, sourcePath, component);
|
||||
}
|
||||
}
|
||||
return table;
|
||||
}
|
||||
|
||||
// Creates a TABLE (for object arrays) or UL (for simple arrays)
|
||||
createHTMLElementForArray(data: unknown[], sourcePath: string, component: Component): HTMLElement | null {
|
||||
async createHTMLElementForArray(data: unknown[], sourcePath: string, component: Component): Promise<HTMLElement | null> {
|
||||
if (data.length === 0) {
|
||||
return null; // Or return null if nothing should be displayed
|
||||
}
|
||||
|
|
@ -169,46 +169,46 @@ export default class YAMLTablePlugin extends Plugin {
|
|||
// Create header
|
||||
const thead = table.createTHead();
|
||||
const headerRow = thead.insertRow();
|
||||
keys.forEach(key => {
|
||||
for (const key of keys) {
|
||||
const th = document.createElement('th');
|
||||
MarkdownRenderer.renderMarkdown(String(key), th, sourcePath, component);
|
||||
await MarkdownRenderer.render(this.app, String(key), th, sourcePath, component);
|
||||
headerRow.appendChild(th);
|
||||
});
|
||||
}
|
||||
|
||||
// Create table body
|
||||
const tbody = table.createTBody();
|
||||
data.forEach(item => {
|
||||
for (const item of data) {
|
||||
if (typeof item === 'object' && item !== null) { // Ensure item is an object
|
||||
const row = tbody.insertRow();
|
||||
const itemRecord = item as Record<string, unknown>; // Cast to access properties safely
|
||||
keys.forEach(key => {
|
||||
for (const key of keys) {
|
||||
const cell = row.insertCell();
|
||||
if (Object.prototype.hasOwnProperty.call(itemRecord, key)) {
|
||||
this.renderValue(itemRecord[key], cell, sourcePath, component);
|
||||
await this.renderValue(itemRecord[key], cell, sourcePath, component);
|
||||
} else {
|
||||
cell.textContent = ''; // Blank for missing keys
|
||||
}
|
||||
});
|
||||
}
|
||||
}
|
||||
// Optionally handle non-object items in the array if needed
|
||||
});
|
||||
}
|
||||
return table;
|
||||
|
||||
} else {
|
||||
// Array of simple values -> create list
|
||||
const list = document.createElement('ul');
|
||||
data.forEach(item => {
|
||||
for (const item of data) {
|
||||
const li = document.createElement('li');
|
||||
this.renderValue(item, li, sourcePath, component); // Render each item
|
||||
await this.renderValue(item, li, sourcePath, component); // Render each item
|
||||
list.appendChild(li);
|
||||
});
|
||||
}
|
||||
return list;
|
||||
}
|
||||
}
|
||||
|
||||
// Renders a value (primitive, object, or array) into a given container element
|
||||
renderValue(value: unknown, container: HTMLElement, sourcePath: string, component: Component) {
|
||||
// Check for Obsidian link pattern parsed as nested array: e.g., [[Link Text]] becomes [['Link Text']]
|
||||
async renderValue(value: unknown, container: HTMLElement, sourcePath: string, component: Component): Promise<void> {
|
||||
// Check for Obsidian link pattern parsed as nested array: e.g., [[Link Text]] becomes [[\'Link Text\']]
|
||||
if (
|
||||
Array.isArray(value) &&
|
||||
value.length === 1 &&
|
||||
|
|
@ -221,7 +221,7 @@ export default class YAMLTablePlugin extends Plugin {
|
|||
) {
|
||||
const linkText = value[0][0];
|
||||
const markdownString = `[[${linkText}]]`;
|
||||
MarkdownRenderer.renderMarkdown(markdownString, container, sourcePath, component);
|
||||
await MarkdownRenderer.render(this.app, markdownString, container, sourcePath, component);
|
||||
return; // Early exit after rendering the link
|
||||
}
|
||||
|
||||
|
|
@ -229,7 +229,7 @@ export default class YAMLTablePlugin extends Plugin {
|
|||
container.textContent = ''; // Render null/undefined as empty
|
||||
} else if (Array.isArray(value)) {
|
||||
// Value is an array -> render as nested table or list
|
||||
const nestedElement = this.createHTMLElementForArray(value, sourcePath, component);
|
||||
const nestedElement = await this.createHTMLElementForArray(value, sourcePath, component);
|
||||
if (nestedElement) {
|
||||
container.appendChild(nestedElement);
|
||||
} else {
|
||||
|
|
@ -238,7 +238,7 @@ export default class YAMLTablePlugin extends Plugin {
|
|||
}
|
||||
} else if (typeof value === 'object' && value !== null) {
|
||||
// Value is an object -> render as nested table
|
||||
const nestedTable = this.createTableFromObject(value as Record<string, unknown>, sourcePath, component);
|
||||
const nestedTable = await this.createTableFromObject(value as Record<string, unknown>, sourcePath, component);
|
||||
if (nestedTable) {
|
||||
// No need to add 'yaml-nested-table' class anymore
|
||||
container.appendChild(nestedTable);
|
||||
|
|
@ -262,13 +262,13 @@ export default class YAMLTablePlugin extends Plugin {
|
|||
}
|
||||
}
|
||||
}
|
||||
MarkdownRenderer.renderMarkdown(markdownString, container, sourcePath, component);
|
||||
await MarkdownRenderer.render(this.app, markdownString, container, sourcePath, component);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Settings tab (No changes needed here for now)
|
||||
class YAMLTableSettingTab extends PluginSettingTab {
|
||||
export class YAMLTableSettingTab extends PluginSettingTab {
|
||||
plugin: YAMLTablePlugin;
|
||||
|
||||
constructor(app: App, plugin: YAMLTablePlugin) {
|
||||
|
|
@ -277,12 +277,12 @@ class YAMLTableSettingTab extends PluginSettingTab {
|
|||
}
|
||||
|
||||
display(): void {
|
||||
const {containerEl} = this;
|
||||
const { containerEl } = this;
|
||||
|
||||
containerEl.empty();
|
||||
containerEl.createEl('h2', {text: 'YAML Table Settings'});
|
||||
|
||||
new Setting(containerEl)
|
||||
.setName('Code block language')
|
||||
.setName('Default Language Name')
|
||||
.setDesc('Language identifier for YAML code blocks to be rendered as tables')
|
||||
.addText(text => text
|
||||
.setPlaceholder('yaml-table')
|
||||
|
|
|
|||
Loading…
Reference in a new issue