Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Dec 19, 2024

Conversation

georgenavarro
Copy link
Contributor

Short description 📝

A recent pull request that modified LaunchAction introduced two issues into the class:

  1. The setter for the pathRunnable property causes infinite recursion
  2. The deprecation notice was attached to the wrong init

Solution 📦

  1. I fixed the infinite recursion in the pathRunnable setter by having it modify the backing runnable property instead of having it accidentally call itself.
  2. I moved the deprecation notice to the init that still accepts a PathRunnable and removed it from the init that accepts a generic Runnable.

Copy link
Collaborator

@kwridan kwridan left a comment

Choose a reason for hiding this comment

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

thanks for catching this @georgenavarro - I missed this during review 🙈

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

// MARK: - Init

@available(*, deprecated, message: "Use the init() that consolidates pathRunnable and runnable into a single parameter.")
public init(runnable: Runnable?,
Copy link
Collaborator

@kwridan kwridan Dec 10, 2024

Choose a reason for hiding this comment

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

this init has both runnable and pathRunnable :/

For the deprecation the following is needed:

// Consolidated init
init(
    runnable: Runnable?,
    // ....
)


@available(*, deprecated, message: "Use the init() that consolidates pathRunnable and runnable into a single parameter.")
init(
    runnable: Runnable?,
    // ....
    pathRunnable: PathRunnable?, // <-- without a default `= nil`,
    // ....
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with this, I'll prepare the change!

Copy link
Collaborator

@kwridan kwridan left a comment

Choose a reason for hiding this comment

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

@pepicrft
Copy link
Contributor

The installation of Swift in the Linux environment is failing. Since I checked that test and build passes locally, I'll go ahead with the merge and investigate that issue separately.
Thanks a lot @georgenavarro once again for your contributions :)

@pepicrft pepicrft merged commit 503473d into tuist:main Dec 19, 2024
3 of 6 checks passed
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.

3 participants