refactor: Update PathRunnable so that it subclasses Runnable #883

georgenavarro · 2024-11-25T01:55:21Z

Short description 📝

PathRunnable currently does not subclass from Runnable like BuildableProductRunnable and RemoteRunnable do. This means that even if you don't care about the specific Runnable implementation, you still have to handle PathRunnable separately from the Runnable subclasses. For example, you would either need two different functions:

func doSomething(with runnable: Runnable)
func doSomething(with runnable: PathRunnable)

or a function that takes in two different parameters:

func doSomething(with runnable: Runnable?, or runnable: PathRunnable?)

Solution 📦

Refactor PathRunnable so that it subclasses from Runnable. This will allow any var or function parameter of type Runnable to hold an instance of PathRunnable. For example the following function would now be able to accept a PathRunnable as an argument in addition to a BuildableProductRunnable or RemoteRunnable:

func doSomething(with runnable: Runnable)

Since the only changes is to make PathRunnable subclass Runnable, this should be a non breaking change. No properties or functions are being added, deleted.

Implementation 👩‍💻👨‍💻

Made the buildableReference parameter of Runnable's init an optional parameter. The buildableReference property of Runnable was already optional so now the types match. Since PathRunnable does not use a BuildableReference, this change will allow PathRunnable to call Runnable's init with a nil BuildableReference argument.
Changed PathRunnable to subclass from Runnable like BuildableProductRunnable and RemoteRunnable do.
Removed PathRunnable's declared conformance to Equatble since it now inherits that from Runnable.
Removed PathRunnable's runnableDebuggingMode property since it now inherits that from Runnable.
Refactored the functions that PathRunnable now overrides from Runnable. I used RemoteRunnable as a model for how to accomplish this.
Removed the pathRunnable property from LaunchAction now that its runnable var can hold an instance of PathRunnable. This avoids the possiblity of having two different runnables, which didn't really make sense. If Xcode detects two runnables, it updates the scheme's launch action so that it only keeps one and deletes the other.
Updated the serialization test case for PathRunnable.

kwridan

Thanks for contributing this @georgenavarro

Looking at the original PR that introduced PathRunnable there doesn't seem to be any specific reason why this wouldn't have been a subclass of Runnable most likely accidental. As far as I can tell there can only be one Runnable, one of which is PathRunnable so makes sense to consolidate this.

#521

That said we'll need a few small tweaks to ensure non breaking changes, perhaps with some deprecation annotations which can be cleaned up in the next major release.

Sources/XcodeProj/Scheme/XCScheme+LaunchAction.swift

kwridan · 2024-11-28T08:35:01Z

Sources/XcodeProj/Scheme/XCScheme+PathRunnable.swift

        public static func == (lhs: PathRunnable, rhs: PathRunnable) -> Bool {
-            lhs.runnableDebuggingMode == rhs.runnableDebuggingMode &&
-                lhs.filePath == rhs.filePath
+            lhs.isEqual(other: rhs) && rhs.isEqual(other: lhs)


Are both checks needed? Would one suffice here?

Suggested change

lhs.isEqual(other: rhs) && rhs.isEqual(other: lhs)

lhs.isEqual(other: rhs)

Yes, I believe your change would be sufficient to check for equality. I did it this way because RemoteRunnable had the same implementation so I thought I should follow the pattern. I'll happily make this change as well. 👍🏼

kwridan · 2024-11-28T08:42:18Z

Sources/XcodeProj/Scheme/XCScheme+PathRunnable.swift

@@ -3,41 +3,47 @@ import Foundation
 import PathKit

 public extension XCScheme {
-    class PathRunnable: Equatable {
+    class PathRunnable: Runnable {


Making PathRunnable subclass Runnable means we'll also need to update ProfileAction to support it

I overlooked this one. I'll try to add this as well.

georgenavarro · 2024-11-29T18:55:43Z

Sources/XcodeProj/Scheme/XCScheme+LaunchAction.swift

+            customLLDBInitFile: String? = nil
+        ) {
+            self.init(
+                runnable: pathRunnable ?? runnable,


I also realized that I removed PathRunnable as a parameter in LaunchAction's init method which would also be a breaking change. I created a convenience init method which adds it back for backwards compatibility. I chose to have it call the designated initializer with it's pathRunnable argument nil coalesced to other runnable argument since it is the more specialized one.

Thanks @georgenavarro

Two small suggestions

Perhaps we can mark it as deprecated and a message to use init(runnable: pathRunnable)

We can make the convenience init have a pathRunnable without a default to help the compiler disambiguate (and to flag the deprecation warning)

I pushed a commit addressing this feedback. Thanks @kwridan

kwridan

thanks for the updates @georgenavarro

kwridan · 2024-12-03T09:52:58Z

Sources/XcodeProj/Scheme/XCScheme+LaunchAction.swift

+            customLLDBInitFile: String? = nil
+        ) {
+            self.init(
+                runnable: pathRunnable ?? runnable,


Thanks @georgenavarro

Two small suggestions

Perhaps we can mark it as deprecated and a message to use init(runnable: pathRunnable)

We can make the convenience init have a pathRunnable without a default to help the compiler disambiguate (and to flag the deprecation warning)

kwridan · 2024-12-03T09:57:03Z

@all-contributors add @georgenavarro for code

allcontributors · 2024-12-03T09:57:13Z

@kwridan

I've put up a pull request to add @georgenavarro! 🎉

…parameter. The buildableReference property of Runnable was optional so now the init parameter type will match. This change will allow PathRunnable, which does not use a BuildableReference to subclass Runnable like BuildableProductRunnable and RemoteRunnable do.

Making PathRunnable subclass from Runnable will bring it line with BuildableProductRunnable and RemoteRunnable. Now all three will now be able to be abstracted behind a Runnable var or parameter.

Removed the dedicated pathRunnable var from LaunchAction now that the runnable var can hold a PathRunnable. Also updated the test case for PathRunnable serialization.

…ackwards compatibility

…arameter for backwards compatibility

georgenavarro · 2024-12-05T02:16:42Z

Sources/XcodeProj/Scheme/XCScheme+LaunchAction.swift

+                runnable as? PathRunnable
+            }
+            set {
+                self.pathRunnable = newValue


@pepicrft Looks like this was supposed to be self.runnable = newValue. Xcode is warning about infinite recursion since it is essentially calling it's own setter.

georgenavarro · 2024-12-05T02:28:16Z

Sources/XcodeProj/Scheme/XCScheme+LaunchAction.swift

@@ -81,6 +87,7 @@ public extension XCScheme {

        // MARK: - Init

+        @available(*, deprecated, message: "Use the init() that consolidates pathRunnable and runnable into a single parameter.")


@pepicrft I think this deprecation warning was attached to the wrong init since the other convenience init now only accepts a PathRunnable

georgenavarro · 2024-12-08T08:27:48Z

I've opened a new PR to fix the issues I mentioned above.

pepicrft · 2024-12-19T09:21:40Z

Ouch, thanks for opening a PR for those issues. Let me review it and get it merged.

kwridan reviewed Nov 28, 2024

View reviewed changes

georgenavarro commented Nov 29, 2024

View reviewed changes

kwridan reviewed Dec 3, 2024

View reviewed changes

allcontributors bot mentioned this pull request Dec 3, 2024

docs: add georgenavarro as a contributor for code #885

Merged

georgenavarro and others added 8 commits December 3, 2024 17:18

Refactored PathRunnable to subclass from Runnable.

8c4f164

Making PathRunnable subclass from Runnable will bring it line with BuildableProductRunnable and RemoteRunnable. Now all three will now be able to be abstracted behind a Runnable var or parameter.

Removed pathRunnable property from LaunchAction

50f4d28

Removed the dedicated pathRunnable var from LaunchAction now that the runnable var can hold a PathRunnable. Also updated the test case for PathRunnable serialization.

Brought back pathRunnable var on LaunchAction as a computed var for b…

3c07bf5

…ackwards compatibility

Removed excessive isEqual check in PathRunnable's == implementation

c6d73d9

Added support for PathRunnable in ProfileAction

7d90c53

Added a convenience init method to LaunchAction with a PathRunnable p…

e656f7d

…arameter for backwards compatibility

Make the inits non-breaking

ad3e91b

pepicrft force-pushed the pathrunnable-inheritance branch from 7681b2f to ad3e91b Compare December 3, 2024 16:38

pepicrft approved these changes Dec 3, 2024

View reviewed changes

pepicrft changed the title ~~Update PathRunnable so that it subclasses Runnable~~ refactor: Update PathRunnable so that it subclasses Runnable Dec 3, 2024

pepicrft merged commit 2297ad2 into tuist:main Dec 3, 2024
5 of 7 checks passed

georgenavarro deleted the pathrunnable-inheritance branch December 3, 2024 18:14

georgenavarro commented Dec 5, 2024

View reviewed changes

georgenavarro mentioned this pull request Dec 8, 2024

fix: Infinite recursion and Incorrect deprecation notice in PathRunnable #889

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Update PathRunnable so that it subclasses Runnable #883

refactor: Update PathRunnable so that it subclasses Runnable #883

georgenavarro commented Nov 25, 2024

kwridan left a comment

kwridan Nov 28, 2024

georgenavarro Nov 29, 2024

kwridan Nov 28, 2024

georgenavarro Nov 29, 2024

georgenavarro Nov 29, 2024

kwridan Dec 3, 2024

pepicrft Dec 3, 2024

kwridan left a comment

kwridan Dec 3, 2024

kwridan commented Dec 3, 2024

allcontributors bot commented Dec 3, 2024

georgenavarro Dec 5, 2024

georgenavarro Dec 5, 2024

georgenavarro commented Dec 8, 2024

pepicrft commented Dec 19, 2024

	lhs.isEqual(other: rhs) && rhs.isEqual(other: lhs)
	lhs.isEqual(other: rhs)

		@@ -81,6 +87,7 @@ public extension XCScheme {

		// MARK: - Init

		@available(*, deprecated, message: "Use the init() that consolidates pathRunnable and runnable into a single parameter.")

refactor: Update PathRunnable so that it subclasses Runnable #883

refactor: Update PathRunnable so that it subclasses Runnable #883

Conversation

georgenavarro commented Nov 25, 2024

Short description 📝

Solution 📦

Implementation 👩‍💻👨‍💻

kwridan 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

kwridan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwridan commented Dec 3, 2024

allcontributors bot commented Dec 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgenavarro commented Dec 8, 2024

pepicrft commented Dec 19, 2024