swiftlang / vscode-swift

Visual Studio Code Extension for Swift
https://marketplace.visualstudio.com/items?itemName=sswg.swift-lang
Apache License 2.0
709 stars 47 forks source link

Show `swift` build status #794

Closed award999 closed 1 month ago

award999 commented 1 month ago

Fixes #694

award999 commented 1 month ago

@adam-fowler @plemarquand I'm adding tests for this but any early feedback would be appreciated

adam-fowler commented 1 month ago

A couple of things

But this is a great demonstration of what we can do with the new SwiftExecution

adam-fowler commented 1 month ago

You are still treating every swift task as if it is a build task. Every task parses for progress, build complete even though they might not be building. You’ve centralized the parsing code around a single SwiftTask class when I think it should be attached to the different instances of tasks. Different swift tasks have different output so they should have their own parsing code.

I also think the multiple layers of Event queues is unnecessary. You are parsing the write event queue to push events to other event queues, is there a reason you can't parse the write event queue to update the UI directly? I know it provides a level of separation, but your API for SwiftTask is defined by the requirements of the build status item. At this point in time I don't see anything else needing progress or build complete events.

You seem to be building for a situation where we are using swift run/test but we don't use swift run in the extension and rarely call swift test. In the cases we do call swift test a swift build has been called beforehand.

I'd much prefer

createSwiftTask(
    ["build", ...additionalArgs],
    buildTaskName,
    config,
    toolchain,
    (line) => {
        UpdateUI(line)
    },
    () => {
        CloseUI()
    }
);

Where the first closure is called on reading a line and the second closure is called on close. You can wrap this in a SwiftBuildTask for tasks that want to parse build output.

adam-fowler commented 1 month ago

If you want to keep this SwiftTask type structure, you could separate out the parsers. So you could create a SwiftTask class which holds an array of parsers. A parser is a bit of code that parses the output and pushes events to a EventEmitter. For your build tasks, you create your SwiftTask then add a progress parser, build complete parser and a fetch parser. For the resolve/update tasks you just add a fetch parser.

award999 commented 1 month ago

I didn't necessarily want to go with the task structure because the VSCode task APIs are a pain, but that was seemingly the middle ground we came to.

You seem to be building for a situation where we are using swift run/test but we don't use swift run in the extension and rarely call swift test

The createSwiftTask function comes into play for the tasks that are executed by commands provided by the extension but does not account for needing to apply this for tasks returned by resolveTask. For example, if running as a preLaunchTask or a task from the tasks.json which get presented when running the "Tasks: Run Task" command. Yes, we could check the args to know which sub-command is running, i.e. "build" or "test", instead of just ignoring the output for other commands. There is also always the option of a task configuration property to turn this on/off. By default we only turn this one for the build commands the extension creates and user could manually turn on if they have a tasks.json configuration. This may not be as discoverable but that may be good enough for now

award999 commented 1 month ago

And @adam-fowler I forgot your other question...

Is there anyway to get the "Building ..." status item and updating the text of that instead of adding a new status item. Seems a little odd having two status items saying Building in the status bar.

Yes. You can call vscode.windows.withProgress and it will allow you to change that "Building..." message and similar to the StatusItem class it will show the latest progress message if you have multiple tasks trying to write. Difference is when clicking the status bar item, it just displays the progress as a notification, whereas StatusItem runs a command to show the running task(s). I will say showing the running command is nice, as a user clicking on a progress message, I may expect to be taken to the running task. On the other hand I agree 2 churning progress status items is going to annoy some people. We cannot even add a button when the progress is expanded to be shown as a notification. To me vscode.windows.withProgress seems cleaner

adam-fowler commented 1 month ago

OK, I see your issue with resolved tasks. If you want to add the parsing to SwiftExecution go for it. I'm not over the moon about it but can't think of a better solution at the moment.

That's so annoying we can update the Building... status item (or at least override it) but lose the ability to go to the terminal where the build is. I noticed internally you can attach a command to a progress status item, don't know why they aren't exposing this. Let's stick with the build status item you have at the moment. Maybe we can remove the building text and just leave as the progress text. Not sure how that would look.

I also added this https://github.com/microsoft/vscode/issues/212963