Support CommandPlugin #449

zunda-pixel · 2022-11-09T09:01:20Z

Add CommandPlugin for format/lint

allevato

Thanks for starting this! I haven't explored the plugin space much yet but this looks useful to have.

I'll caution you that the previous time I tried adding a plugin, we had to revert it because of CI and Windows-related failures: #424. But that was a build tool plugin that was generated sources to use during swift-format's own build, so that might have been different. Either way, once this is in shape, we'll see if CI has any problems with it. 🤞🏻

allevato · 2022-11-09T17:02:13Z

Package.swift

@@ -33,6 +33,14 @@ let package = Package(
      name: "SwiftFormatConfiguration",
      targets: ["SwiftFormatConfiguration"]
    ),
+    .plugin(
+      name: "Format Source Code",


I think we should remove the spaces from the target names and subdirectory names. Names like FormatPlugin and LintPlugin would align well with the naming we've used elsewhere.

For the name argument of the .plugin, I'm less sure. I believe this surfaces in the Xcode UI? Is there anywhere else that this string is used (like a command line) where spaces would cause problems?

I fixed with reference to swift-argument-parser

zunda-pixel/swift-format/pull/1

Xcode uses Package.targets.plugin.name as UI

allevato · 2022-11-09T17:03:55Z

Package.swift

+    .plugin(
+      name: "Lint Source Code",
+      capability: .command(
+        intent: .sourceCodeFormatting(),


What's the behavior of swift package format-source-code when there are two plugins with this intent?

I wonder if we should use something like .custom(verb: "lint-source-code") here instead.

I fixed it. zunda-pixel@8bcb0bb

swift-format$ swift package plugin --list ‘format-source-code’ (plugin ‘Format Source Code’ in package ‘swift-format’) ‘generate-manual’ (plugin ‘Generate Manual’ in package ‘swift-argument-parser’) ‘lint-source-code’ (plugin ‘Lint Source Code’ in package ‘swift-format’)

allevato · 2022-11-09T17:04:39Z

Plugins/Format Source Code/plugin.swift

+      var swiftFormatArgs = [
+        "format",
+        target.directory.string,
+        "-r",


Here and throughout, can you use the --long-form of the flags instead, so it's easier for a reader to understand when they quickly scan it?

I don't know --long-form. what is this?

Every flag that has a short form (like -r) has a longer form (--recursive). Using the longer ones will make the usage clearer. They're documented here: https://github.com/apple/swift-format#command-line-usage

allevato · 2022-11-09T17:16:03Z

Plugins/Format Source Code/plugin.swift

+        "-i",
+      ]
+
+      if let configuration {


Here and elsewhere, let's avoid using new Swift 5.7 syntax for now; Package.swift still specifies a 5.6 tools version, and I'm not sure how it plays with CI (some other projects avoid it).

I fixed it. zunda-pixel@60c66fc

allevato · 2022-11-09T17:30:03Z

Plugins/Format Source Code/plugin.swift

+
+    let configuration = argExtractor.extractOption(named: "configuration").first
+
+    for target in targetsToFormat {


Instead of spawning swift-format once for each target, it would be more efficient to collect the directory paths in an array and then pass them all at once in the args array to just do a single invocation.

I fixed It.

allevato · 2022-11-09T17:34:19Z

Plugins/Format Source Code/plugin.swift

+
+extension FormatSourceCode: XcodeCommandPlugin {
+  func performCommand(context: XcodeProjectPlugin.XcodePluginContext, arguments: [String]) throws {
+    let swiftFormatTool = try context.tool(named: "swift-format")


This code is mostly duplicated from the function above. Can you factor out the commonalities into a file-scoped function and just have the plugins call that?

I think a single function that takes the tool returned by context.tool, the arguments, and the list of directories to format would do the trick.

I made func format(tool: PluginContext.Tool, targetDirectories: [String], configurationFilePath: String?) throws
https://github.com/zunda-pixel/swift-format/blob/36c8e5bff9a01cb96c5f013ca021f79263f13ea7/Plugins/FormatPlugin/plugin.swift#L6

allevato · 2022-11-09T17:49:00Z

Plugins/Format Source Code/plugin.swift

+    let swiftFormatExec = URL(fileURLWithPath: swiftFormatTool.path.string)
+    var swiftFormatArgs = [
+      "format",
+      ".",


From the XcodePluginContext you can reach through context.xcodeProject.directory, which might be safer than assuming the working directory of the plugin will always be the project directory.

I fixed it. zunda-pixel@2fee7b9

allevato · 2022-11-09T17:50:30Z

Plugins/Lint Source Code/plugin.swift

@@ -0,0 +1,92 @@
+import PackagePlugin


Same comments from the Format Source Code file apply here (factoring out the common implementations, making a single invocation to swift-format, accessing the Xcode project directory).

allevato · 2022-11-16T15:58:49Z

Plugins/FormatPlugin/plugin.swift

+
+#endif
+
+@resultBuilder


This feels like overkill; I don't think it improves the readability much. Let's just use regular array construction/appending above; it's more obvious to the reader what's going on without having to look for the builder (which has to be repeated in both plugins).

allevato · 2022-11-16T16:00:53Z

Plugins/Format Source Code/plugin.swift

+      var swiftFormatArgs = [
+        "format",
+        target.directory.string,
+        "-r",


Every flag that has a short form (like -r) has a longer form (--recursive). Using the longer ones will make the usage clearer. They're documented here: https://github.com/apple/swift-format#command-line-usage

allevato · 2022-11-16T16:02:23Z

Plugins/FormatPlugin/plugin.swift

+
+    try format(
+      tool: swiftFormatTool,
+      targetDirectories: sourceCodeTargets.map(\.directory).map(\.string),


Can this be combined into a single map call, map(\.directory.string)? That would avoid unnecessary array allocations.

Same in the lint plugin.

I fixed it. 28f9876

allevato

Thank you! Waiting for CI results, which will be reported at swiftlang/swift-syntax#1086.

allevato · 2022-11-17T23:02:57Z

CI looks good. Can you squash the commits and re-push them so we can keep the history (even after merge) a bit cleaner?

zunda-pixel · 2022-11-18T01:36:17Z

I cleaned up git history.
I don't know git squash well, Is it ok?

allevato

This looks great; thank you again for the contribution!

zunda-pixel mentioned this pull request Nov 9, 2022

Support CommandPlugin #447

Closed

allevato reviewed Nov 9, 2022

View reviewed changes

zunda-pixel requested a review from allevato November 11, 2022 06:12

allevato reviewed Nov 16, 2022

View reviewed changes

zunda-pixel requested a review from allevato November 17, 2022 00:44

allevato mentioned this pull request Nov 17, 2022

[DO NOT MERGE] allevato's dummy PR for swift-format CI swiftlang/swift-syntax#1086

Open

allevato approved these changes Nov 17, 2022

View reviewed changes

Add Format/Lint Source Code Plugin

05aa376

zunda-pixel force-pushed the main branch from 51c0770 to 05aa376 Compare November 18, 2022 01:32

zunda-pixel requested a review from allevato November 18, 2022 04:54

allevato approved these changes Nov 18, 2022

View reviewed changes

allevato merged commit 949c36d into swiftlang:main Nov 18, 2022

kamcma mentioned this pull request Dec 15, 2022

Make some space pointfreeco/swift-composable-architecture#1747

Closed

uhooi mentioned this pull request Jan 5, 2023

Add Command Plugins realm/SwiftLint#4671

Open

shimastripe mentioned this pull request Mar 9, 2024

Package plugin cannot be used if dependency versions are unstable #699

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support CommandPlugin #449

Support CommandPlugin #449

zunda-pixel commented Nov 9, 2022 •

edited

Loading

allevato left a comment

allevato Nov 9, 2022

zunda-pixel Nov 10, 2022

allevato Nov 9, 2022

zunda-pixel Nov 10, 2022

allevato Nov 9, 2022

zunda-pixel Nov 10, 2022

allevato Nov 16, 2022

allevato Nov 9, 2022

zunda-pixel Nov 10, 2022

allevato Nov 9, 2022

zunda-pixel Nov 10, 2022

allevato Nov 9, 2022

zunda-pixel Nov 10, 2022

allevato Nov 9, 2022

zunda-pixel Nov 10, 2022 •

edited

Loading

allevato Nov 9, 2022

allevato Nov 16, 2022

allevato Nov 16, 2022

allevato Nov 16, 2022

zunda-pixel Nov 17, 2022

allevato left a comment

allevato commented Nov 17, 2022

zunda-pixel commented Nov 18, 2022

allevato left a comment


		let configuration = argExtractor.extractOption(named: "configuration").first

		for target in targetsToFormat {


		#endif

		@resultBuilder

Support CommandPlugin #449

Support CommandPlugin #449

Conversation

zunda-pixel commented Nov 9, 2022 • edited Loading

allevato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zunda-pixel Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allevato left a comment

Choose a reason for hiding this comment

allevato commented Nov 17, 2022

zunda-pixel commented Nov 18, 2022

allevato left a comment

Choose a reason for hiding this comment

zunda-pixel commented Nov 9, 2022 •

edited

Loading

zunda-pixel Nov 10, 2022 •

edited

Loading