Skip to content

Conversation

@amanthatdoescares
Copy link

@amanthatdoescares amanthatdoescares commented Dec 17, 2025

Description

This change adds a new Swift command that helps users get started with DocC documentation.

The command prompts for a module name and creates a basic DocC catalog in the workspace, including:

  • A <Module>.docc directory
  • An initial markdown file with the module title

This removes the need to manually set up the DocC folder structure and makes it easier to start documenting Swift packages directly from VS Code.

Issue: #1647

Tasks

  • Required tests have been written
  • Documentation has been updated (not applicable)
  • Added an entry to CHANGELOG.md if applicable

Copy link
Member

@matthewbastien matthewbastien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR! There are a few issues that need to be resolved before this can be merged.


if (hasPackageSwift) {
try {
const { stdout } = await execFileAsync("swift", ["package", "dump-package"], {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This information is already computed and stored on the folderContext.swiftPackage for each Swift Package folder open in the workspace. See https://github.com/swiftlang/vscode-swift/blob/main/src/commands.ts#L80 for a reference of how to get the folderContext for the currently active folder and pass it in to the command.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing this out — I wasn’t aware this information was already available via FolderContext. I’ll refactor the command to use folderContext.swiftPackage instead of invoking swift package dump-package.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

for (const name of targets) {
const srcPath = path.join(rootPath, "Sources", name);
try {
await fs.access(srcPath);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use fileExists in utilities/filesystem instead

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

once i read filesystem i think folderExists is much more accurate in this case

Copy link
Member

@matthewbastien matthewbastien Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fileExists() and folderExists() check for the path in question to be a file or folder respectively. You're going to want to use pathExists() because you can't create the folder if anything exists at that path regardless of whether or not it is a file, folder, symlink, etc.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matthewbastien Target discovery still uses folderExists() since those must be directories.


const doccDir = path.join(basePath, `${value}.docc`);
try {
await fs.access(doccDir);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fileExists here as well

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think folderExists is much accurate here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll have to use pathExists().

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, i have made that commit now

@matthewbastien
Copy link
Member

I'd also like to have an integration test for this since the logic will be interacting with the FolderContext. Something that runs the command and makes sure that all of the targets show up correctly in the quick pick. It should also make sure that the catalog gets created properly.

@amanthatdoescares
Copy link
Author

@matthewbastien I’ve added an integration test that runs the command end-to-end, verifies SwiftPM targets appear in the QuickPick, and asserts the DocC catalog is created on disk.

This is my first time writing an integration test in this codebase, so I’d really appreciate a review on the test structure and coverage.

},
});

test("creates a DocC catalog for a SwiftPM target", async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: The nested test() is unnecessary and will make it so that this test never actually runs.

import { tag } from "../../tags";
import { activateExtensionForSuite, folderInRootWorkspace } from "../utilities/testutilities";

tag("large").suite("Create Documentation Catalog Command", function () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: This suite should be marked as either "medium" or "small". It definitely doesn't need 10 minutes just to create a directory. I'm leaning towards "small" in this instance since it really shouldn't take long at all.

//
// Copyright (c) 2025 the VS Code Swift project authors
// Licensed under Apache License v2.0
//
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: This is missing part of the copyright header.

// This source file is part of the VS Code Swift open source project
//
// Copyright (c) 2025 the VS Code Swift project authors
// Licensed under Apache License v2.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: This is missing part of the copyright header.

const basePath = selection.basePath;

// ---- module name input ----
const moduleName = await vscode.window.showInputBox({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: By convention the module name for a target should match the target name if the user selected one in the previous step. The extension should only ask for a folder if the user selected a standalone documentation catalog.

expect(await fs.stat(markdownFile)).to.exist;

const contents = await fs.readFile(markdownFile, "utf8");
expect(contents).to.contain("# MyModule");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: The module name when selecting a target should match the module name of that target.

}
});
});
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: We should also test the logic for creating a standalone documentation catalog.


test("creates a DocC catalog for a SwiftPM target", async () => {
test("creates a DocC catalog for a SwiftPM target", async () => {
const quickPickStub = sinon.stub(vscode.window, "showQuickPick");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: We have methods in MockUtils.ts that will do this for you using mocha's setup() and teardown() blocks. You don't need to use sinon.stub() or the try {} finally {} here. For example:

import { mockGlobalObject } from "../../MockUtils.ts";

suite("some-suite", () => {
    const windowMock = mockGlobalObject(vscode, "window");

    test("some-test", () => {
        windowMock.showQuickPick.resolves("blah");
    });
});

const inputBoxStub = sinon.stub(vscode.window, "showInputBox");

try {
inputBoxStub.resolves("MyModule");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: The module name quick pick should not be shown if selecting a target.

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.

4 participants