Return a non-zero exit code when SwiftFormat autocorrect applies code changes #268

calda · 2024-04-16T19:40:38Z

This PR updates the SPM package plugin to return a non-zero exit code when SwiftFormat autocorrect applies code changes, using the new --strict flag added in nicklockwood/SwiftFormat#1676.

This matches the behavior of our internal linting tool.

… changes

bachand · 2024-04-17T21:33:19Z

Package.swift

@@ -42,8 +42,8 @@ let package = Package(

    .binaryTarget(
      name: "SwiftFormat",
-      url: "https://github.com/calda/SwiftFormat/releases/download/0.54-beta-5/SwiftFormat.artifactbundle.zip",
-      checksum: "7447986db45a51164d23672c07f971406a4c0589b0c423fcb85e95ed8f8e7e48"),
+      url: "https://github.com/calda/SwiftFormat/releases/download/0.54-beta-6/SwiftFormat.artifactbundle.zip",


I checked that this release includes the --strict change.

bachand · 2024-04-17T21:33:34Z

Package.swift

-      url: "https://github.com/calda/SwiftFormat/releases/download/0.54-beta-5/SwiftFormat.artifactbundle.zip",
-      checksum: "7447986db45a51164d23672c07f971406a4c0589b0c423fcb85e95ed8f8e7e48"),
+      url: "https://github.com/calda/SwiftFormat/releases/download/0.54-beta-6/SwiftFormat.artifactbundle.zip",
+      checksum: "2b7213c898f0914713b5d8a75e4ed7c560552f3a9e69f31dbcd38c99b878d738"),


@calda how do you find/compute this value? Asking to learn.

I usually:

Update the url to point to the new value

Run rm -rf .build && swift package format

SPM will print out an error that includes the old checksum and the new checksum

Copy the new checksum into the package manifest

There's also a command to calculate this yourself but I don't know it off the top of my head so use this lazy method instead 😛

in case it's helpful: if you have the zip locally, you can always run xcrun swift package compute-checksum <path_to_zip_file>

bachand

@calda thanks for continuing to improve this tool! I was about to approve this PR but now I am realizing that I am unclear on what the behavior of our tool should be. Should the tool fail or succeed if there were violations that were automatically corrected? My understanding was that we wanted the tool to succeed if the violations were automatically corrected, which is the current behavior when running SwiftLint (see testFormatWithOnlySwiftLintAutocorrectedViolation()).

I am also recalling that we had a similar discussion previously in #242 (comment). In that discussion we aligned that we did not want the tool to fail with a nonzero exit code when errors could be automatically corrected. It seems like we are changing that behavior now, but only for SwiftFormat?

~~In that previous discussion I suggested:~~

~~What do you think about documenting the exit code behavior of this tool in the README as part of this PR and then we can ensure that the behavior matches the documented behavior?~~

Personally, I think we should consider taking on that work as part of this PR. I am finding it challenging to review this PR without knowing what the expected behavior is. Moreover, I think this tool that we are building will be easier for others to adopt if we have documentation on how it is meant to function.

Update: We already updated the README with the expected behavior! 🎉 😍 I am sorry that I did not dig deeper and notice when I was writing this comment originally 🙇‍♂️

Based on this update documented behavior, though, it seems like we do not want to add the --strict flag 🤔 I am curious to get your thoughts @calda .

bachand · 2024-04-17T21:36:18Z

Tests/AirbnbSwiftFormatToolTests/AirbnbSwiftFormatToolTests.swift

+          // When autocorrecting, SwiftFormat returns EXIT_SUCCESS even if there were
+          // violations that were fixed, unless you pass in the --strict option.
+          if command.arguments.contains("--strict") {
+            return SwiftFormatExitCode.lintFailure


bachand · 2024-04-17T21:41:27Z

Tests/AirbnbSwiftFormatToolTests/AirbnbSwiftFormatToolTests.swift

          ranSwiftLintAutocorrect = true

          // When autocorrecting SwiftLint returns EXIT_SUCCESS
          // even if there were violations that were fixed
          return EXIT_SUCCESS
        }))

-    XCTAssertEqual(error as? ExitCode, ExitCode(SwiftFormatExitCode.lintFailure))
+    XCTAssertEqual(error as? ExitCode, .failure)


Is this the definition of ExitCode? https://apple.github.io/swift-argument-parser/documentation/argumentparser/exitcode/issuccess

If so, I feel like it may be better to check isSuccess than test against .failure. Will this check assertion be satisfied for any nonzero failing exit code?

bachand · 2024-04-17T21:43:29Z

Tests/AirbnbSwiftFormatToolTests/AirbnbSwiftFormatToolTests.swift

+          return EXIT_SUCCESS
+        }))
+
+    XCTAssertEqual(error as? ExitCode, .failure)


This is a change in behavior, if I am understanding correctly.

calda · 2024-04-17T22:10:22Z

@bachand -- this is a change in behavior to match our internal lint:swift command, which returns a non-zero exit code if SwiftFormat autocorrect applies any changes.

I was thinking it would make sense for this public tool to match the behavior of our internal tool, especially since we just added this --strict option to SwiftFormat for that use case.

You're right that we should also update the README, thanks for noticing. I'll make that change.

Do you think this change directionally makes sense, or do you think we should keep the current behavior where autocorrect mode will return a success / zero exit code when applying changes?

bachand · 2024-04-17T22:25:31Z

@bachand -- this is a change in behavior to match our internal lint:swift command, which returns a non-zero exit code if SwiftFormat autocorrect applies any changes.

I was thinking it would make sense for this public tool to match the behavior of our internal tool, especially since we just added this --strict option to SwiftFormat for that use case.

You're right that we should also update the README, thanks for noticing. I'll make that change.

Do you think this change directionally makes sense, or do you think we should keep the current behavior where autocorrect mode will return a success / zero exit code when applying changes?

@calda personally I think that documented behavior of this tool, returning 0 success code when any violations can be automatically corrected, makes sense. The motivation for returning a non-0 failure exit code when autocorrections are made, as I understand, is so that we can fail Git pre-commit / pre-push hooks if there are automatically corrected violations that were made and now need to be committed. My personal belief is that by default this tool should return a successful code "when there is no action for the user to take", either because there were no violations or they have been automatically fixed. This aligns with the default behavior of SwiftFormat/SwiftLint, at least as of the last time that I tested those tools.

That all being said, I think it would be reasonable to add a "strict" mode to our tool so that this tool can be used in a Git pre-commit / pre-push hook and fail that hook when autocorrections have been made. However, I think the strict mode should be opt-in.

I also think that it's important that the behavior of this tool is consistent. My understanding of this change is that the tool will fail if there's a SwiftFormat autocorrection but not if there is a SwiftLint autocorrection. I think that it's important that this tool has the same behavior for both "modes" of autocorrection.

calda · 2024-04-17T22:35:24Z

Sounds good, thanks for sharing your thoughts. Let's leave this tool as-is for now then!

Return a non-zero exit code when SwiftFormat autocorrect applies code…

da4e50c

… changes

calda force-pushed the cal--swiftformat-strict branch from 00abb6e to da4e50c Compare April 16, 2024 19:43

bachand reviewed Apr 17, 2024

View reviewed changes

calda closed this Apr 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return a non-zero exit code when SwiftFormat autocorrect applies code changes #268

Return a non-zero exit code when SwiftFormat autocorrect applies code changes #268

calda commented Apr 16, 2024

bachand Apr 17, 2024

bachand Apr 17, 2024

calda Apr 17, 2024

dfed Apr 18, 2024

bachand left a comment •

edited

bachand Apr 17, 2024

bachand Apr 17, 2024

bachand Apr 17, 2024

calda commented Apr 17, 2024 •

edited

bachand commented Apr 17, 2024 •

edited

calda commented Apr 17, 2024

Return a non-zero exit code when SwiftFormat autocorrect applies code changes #268

Return a non-zero exit code when SwiftFormat autocorrect applies code changes #268

Conversation

calda commented Apr 16, 2024

bachand Apr 17, 2024

Choose a reason for hiding this comment

bachand Apr 17, 2024

Choose a reason for hiding this comment

calda Apr 17, 2024

Choose a reason for hiding this comment

dfed Apr 18, 2024

Choose a reason for hiding this comment

bachand left a comment • edited

Choose a reason for hiding this comment

bachand Apr 17, 2024

Choose a reason for hiding this comment

bachand Apr 17, 2024

Choose a reason for hiding this comment

bachand Apr 17, 2024

Choose a reason for hiding this comment

calda commented Apr 17, 2024 • edited

bachand commented Apr 17, 2024 • edited

calda commented Apr 17, 2024

bachand left a comment •

edited

calda commented Apr 17, 2024 •

edited

bachand commented Apr 17, 2024 •

edited