Skip to content

Improve tsdocs #176

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

Open
wants to merge 152 commits into
base: master
Choose a base branch
from
Open

Improve tsdocs #176

wants to merge 152 commits into from

Conversation

mnaoumov
Copy link
Contributor

No description provided.

@mnaoumov mnaoumov marked this pull request as draft March 13, 2025 04:17
@mnaoumov mnaoumov closed this Mar 13, 2025
@mnaoumov
Copy link
Contributor Author

Created accidentally. It meant to be mnaoumov#1

@mnaoumov mnaoumov marked this pull request as ready for review March 15, 2025 05:31
@mnaoumov mnaoumov requested a review from joethei March 15, 2025 05:31
@gfxholo
Copy link

gfxholo commented May 7, 2025

Love the sheer amount of effort that you've put in here! 💜 I do think this pull request could be improved by focusing on the quality of documentation more than the quantity, though. These changes could be trimmed down to just the methods and properties which you possess useful knowledge about.

Here, these examples below represent a pattern of style I'm seeing inside a large chunk of your additions. Docs like these won't help a plugin developer understand the API any better than just reading the method and parameter names, and might actually create reading fatigue.

(This is just a general critique! You don't have to respond to each of these examples 🙏)

Setting class

/**
 * The HTML element for the info.
 *
 * @public
 */
infoEl: HTMLElement;

/**
 * The HTML element for the control.
 *
 * @public
 */
controlEl: HTMLElement;

These descriptions are pretty vague. What is info? That sounds like it could be the setting description. What does control point to, since settings can have multiple controls?

/**
 * Add a moment format component to the setting.
 *
 * @param cb - The callback to add the moment format component.
 * @returns The setting.
 *
 * @example
 * ```ts
 * setting.addMomentFormat((momentFormat) => {
 *     momentFormat.setValue('YYYY-MM-DD');
 * });
 * ```
 *
 * @public
 */
addMomentFormat(cb: (component: MomentFormatComponent) => any): this;

This component definitely needs good descriptions, but this one doesn't provide any explanation of what a moment format does (though you have written a good docstring for MomentFormatComponent).

/**
 * Clear the setting.
 *
 * @returns The setting.
 *
 * @example
 * ```ts
 * setting.clear();
 * ```
 *
 * @public
 */
clear(): this;

What does this clear, exactly? Does it reset the setting to its default? Will it clear all my text components to ""?

View class

/**
 * The type of the view.
 *
 * @returns The type of the view.
 *
 * @public
 */
abstract getViewType(): string;

What is a view type? Is it unique? What should it look like? What is it used for? How do core plugins name their types?

/**
 * Get the display text of the view.
 *
 * @returns The display text of the view.
 *
 * @public
 */
abstract getDisplayText(): string;

What is display text, exactly? Where will it be displayed?

/**
 * Set the state of the view.
 *
 * @param state - The state of the view.
 * @param result - The result of the view.
 *
 * @returns A promise that resolves when the state is set.
 *
 * @example
 * ```ts
 * this.setState({ foo: 'bar' }, { history: true });
 * ```
 *
 * @public
 */
setState(state: unknown, result: ViewStateResult): Promise<void>;

State is a very generic word in programming, so what does this method actually do? What makes state different to ephemeral state? What's inside the result? Should we call this method ourselves, or just override it?

Workspace class

/**
 * The root split of the workspace.
 *
 * @public
 */
rootSplit: WorkspaceRoot;

This is another "rewriting the method name as a sentence" example. What is a root split? If I call getRoot() on a sidebar leaf, is this what it returns?

/**
 * Change the layout of the workspace.
 *
 * @param workspace - The workspace to change the layout to.
 * @returns A promise that resolves when the layout is changed.
 *
 * @public
 */
changeLayout(workspace: any): Promise<void>;

Because workspace is an untyped object, how does the developer even know what to pass in here? Some clarification is sorely needed.

/**
 * Open a link text.
 *
 * @param linktext - The link text to open.
 * @param sourcePath - The source path to open.
 * @param newLeaf - The type of the leaf to open.
 * @param openViewState - The view state to open.
 *
 * @example
 * ```ts
 * app.workspace.openLinkText('foo', 'bar.md', 'tab');
 * ```
 *
 * @public
 */
openLinkText(linktext: string, sourcePath: string, newLeaf?: PaneType | boolean, openViewState?: OpenViewState): Promise<void>;

There's a lot happening in this method, and the documentation doesn't explain any of it. Is a link text like a URL? If so, what makes link text different to sourcePath? And what does the optional newLeaf and openViewState do?

@mnaoumov
Copy link
Contributor Author

mnaoumov commented May 8, 2025

@gfxholo I agree with all the critique. Feel free to send PRs to my tsdocs here or to https://github.com/Fevol/obsidian-typings, where I made those docs more structured

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.

3 participants