Break up the massive `installPackage` function about cork HOT 20 OPEN

Here's a link to the function in question:

https://github.com/buresdv/Cork/blob/main/Cork/Logic/Installation%20%26%20Removal/Installation/Install%20Selected%20Packages.swift

from cork.

Ah this explains the packagesBeingInstalled[0] all over the place! Ok I will ignore. Fixing this by moving more logic out out of the views and into the ObservedObjects could also help a lot too. But I'm going to leave that.

from cork.

I'm reading up on the in-depths of concurrency to hopefully fix it soon

This can be tough, but please do let me know what you are reading and how it goes! This is an active project of mine.

from cork.

Ah, I know it and I have already read a few chapters! I was slightly worried, because Donny wrote it before Swift 5.10 shipped and that version contains major changes related to concurrency. But I really should finish it up!

from cork.

Ok InstallationProgressTracker has zero logic. I think it may make sense to move stuff in there.

from cork.

One easy way to recognize this is:

• The function is used in only one place
• The function takes an object as an argument

This feels like a method! The usage looks like this:

let installationResult = try await installPackage(installationProgressTracker: installationProgressTracker, brewData: brewData)

With method it would look more like this:

let installationResult = try await installationProgressTracker.installPackage(using: brewData)

What do you think?

from cork.

Hmm that's an interesting idea! I think it could work. The current architecture is a bit unconventional (most of the logic is completely detached from objects, with some exceptions like BrewDataStorage and OutdatedPackageTracker). I was thinking of gradually making those single-use functions into methods

from cork.

Making it a top-level function does come with a number of major drawbacks. One is the mutation of the internal state of installationProgressTracker is really awkward, motivating the use of things like installationProgressTracker.packagesBeingInstalled[0]. The other is the logic and the type the logic operates on are in different places.

This isn't inherently wrong! And I want to leave stylistic things like this up to you. It is possible to improve this with more top-level functions as well.

from cork.

Thanks for the insight! You're right, the mutation of internal state is one of the motivations I had for adding methods to BrewDataStorage etc.

Truth be told, Cork is my second ever programming project, so it has a lot of weirdness stemming from me having no idea what I was doing back then 😂 I'm always happy to learn, so if you think making it a method would make it more convenient, I'm all for it. I just don't want to make massive changes all over the place, which I would not be able to maintain. But I'm okay with a few changes here and there.

from cork.

I think you'll find that not making structural improvements over time will lead to a higher maintenance burden - like this current issue!

The secret is just to go slowly.

from cork.

Yes, you're absolutely correct 😊

from cork.

Ok I've begun a PR: #295

from cork.

I think I may have found a bug though:

line 104 of "Installing.swift"

for var packageToInstall in installationProgressTracker.packagesBeingInstalled

packageToInstall is unused?

from cork.

Oh yeah, this is a vestige from WAY back when there was still support for installing more than one package at the same time, which is not coming back. I've been meaning to completely remove the packagesBeingInstalled property and merge it into packageBeingCurrentlyInstalled (which is always PackageInProgressOfBeingInstalled.name, anyway ), creating a new property packageBeingInstalled, like so:

class InstallationProgressTracker: ObservableObject
{
    @Published var packageBeingInstalled: PackageInProgressOfBeingInstalled = .init()

    @Published var numberOfPackageDependencies: Int = 0
    @Published var numberInLineOfPackageCurrentlyBeingFetched: Int = 0
    @Published var numberInLineOfPackageCurrentlyBeingInstalled: Int = 0
}

from cork.

So in the end, that loop isn't needed anymore, since there can now be only one package being installed at a time.

from cork.

I'm also now noticing some real concurrency issues. In particular, it almost never makes sense to have an ObservedObject that is not MainActor-isolated.

Using targeted checking is a good start! But, this refactor may expose more issues. I'm also not going to address this right now, but it's definitely something you'll need to think about.

from cork.

You're right! That's definitely something I'm planning. There are a lot of hidden concurrency issues (like #190, I suspect). I'm reading up on the in-depths of concurrency to hopefully fix it soon 😊

from cork.

I'm reading this: https://donnywals.gumroad.com/l/practical-swift-concurrency and I'd definitely recommend it!

from cork.

Ok, I know this is closed, but I thought I'd just throw some more ideas in here. We did the easy stuff: break up a big if-else into two functions and make use of an instance method instead of a top-level function.

But the two remaining functions are still quite large. And now it's harder to simplify. I think there are two things contributing to the verbosity: the logging and the string pattern matching. One idea might be to move the pattern matching into an enum-based system. This might make testing a little easier too. It's possible that using an enum could also internalize some of the logging. Would have to experiment a little to see how that works.

But, my next area of investigation would be changing all of the conditionals like if outputLine.contains("Downloading") into an enum initializer and going from there.

from cork.

No problem :) I'll reopen this.

I originally wanted to have the matching to be done in an enum, but I couldn't figure out how to do it. You can see if you can figure something out, I'd be really grateful.

from cork.