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

Create dedicated html outputShortcut #2769

Merged
merged 1 commit into from
Nov 10, 2024

Conversation

tgreyuk
Copy link

@tgreyuk tgreyuk commented Nov 10, 2024

@Gerrit0 Thanks for the great work on the beta release. I have been working against it to prepare typedoc-plugin-markdown when it lands. I hope this PR makes sense but let me know what you think.

Summary / Purpose

While updating typedoc-plugin-markdown to properly utilize the "outputs" implementation as per #2597 (which is fantastic by the way) I have stumbled upon a bit on an issue.

The issue is that the "html" outputShortcut is bound to the "out" declaration. By reading the above thread I think the initial intention was perhaps to rename the option?

"The --out option will be replaced with a --html option.".

I understand that this is a significant breaking change, so I see the importance of preserving the out option. Ideally, though, I would prefer to avoid introducing a plugin breaking change of requiring a "markdown" option to be specified (instead of the generic "out" option) in order to generate the markdown docs. As it stands there is no way around this.

Proposal

I propose to preserve the existing "out" option as a basic path declaration and to introduce a dedicated "html" outputShortcut. To the end user this will have no effect, as the Outputs class will assume that no shortcut has been specified and apply the html output as the default output.

However this also means that I can create a markdown output and set it as the default and preserve the existing behaviour of the "out" option.

ie something like this:

app.options.addDeclaration({
    name: 'markdown',
    outputShortcut: 'markdown',
    help: (i18n) => i18n.help_markdown(),
    type: ParameterType.Path,
    hint: ParameterHint.Directory,
    defaultValue: './docs',
  });

app.outputs.addOutput('markdown', async (out, project) => {
    await customMarkdownRenderer(out, project)
  });

  app.outputs.setDefaultOutput(() => {
    return {
      name: 'markdown',
      path: app.options.getValue('out'),
    };
  });

Then when outputs are specified in config, the dedicated shortcuts will be picked up so the existing behaviour is preserved here too:

 "outputs": [
        {
            "name": "html",
            "path": "../html-docs"
        },
        {
            "name": "markdown",
            "path": "../markdown-doc",
        }
    ],

Changes

The changes are pretty simple:

  • removed outputShortcut key from the "out" option.
  • added a dedicated "html" outputShortcut
  • updated help text (put the translation through chat gpt so not sure of the accuracy)

@tgreyuk tgreyuk force-pushed the fix/dedicated-html-outputshortcut branch from 57be65a to ffc6806 Compare November 10, 2024 07:39
@tgreyuk tgreyuk force-pushed the fix/dedicated-html-outputshortcut branch from ffc6806 to 4c2c106 Compare November 10, 2024 07:42
@Gerrit0
Copy link
Collaborator

Gerrit0 commented Nov 10, 2024

Thanks, this is a good idea! However, the implementation doesn't quite work, as now typedoc --out docs --json docs.json will only produce JSON output.

Making --out dedicated to the default output makes the output class slightly simpler. The setDefaultOutput method can be replaced with one which just sets the default output name rather than a callback. Doing this will prevent accidental misuse by plugins which get a different option than out.

I'm also going to revert the changes to the non-english locales -- my policy there is not to include machine generated translations as I've seen bad output from them before... which I thought I'd put somewhere obvious, but apparently have not.

@Gerrit0 Gerrit0 merged commit 4c2c106 into TypeStrong:beta Nov 10, 2024
5 checks passed
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