XcodesOrg / xcodes

The best command-line tool to install and switch between multiple versions of Xcode.
MIT License
3.66k stars 122 forks source link

Update the cached list of available Xcodes if it's more than 24 hours old #226

Closed rpendleton closed 1 year ago

rpendleton commented 1 year ago

This fixes #202, although I made a few assumptions that I'm open to discussion about.

Specifically:

  1. I wasn't sure how widespread the cache updating should be. My changes hook in to the existing shouldUpdate property, but I could see value in doing this differently depending on the scenario. (For example, you might not need to update the list of Xcodes to install a version that's already known. That would be more complex though.)

  2. I wasn't sure whether it made sense to change the format of the cache file (and whether that would need migrations), so rather than doing that, I decided to just check modification dates of the cache file on disk. An alternative might be to store the modification date in the cache file or some other configuration/preference file.

  3. Although it's unlikely, reading the attributes of a file can technically fail. If that happens (or if the modified date is otherwise missing), I made it so the cached list will load anyway, and shouldUpdate will return false. I'm not sure whether this makes the most sense, but I figured it was better than repeatedly requesting data from the server.

If any of those assumptions seem like bad ones, I'm open to alternatives.

rpendleton commented 1 year ago

In my first assumption, I mentioned that an alternative for xcodes install ... might be to only update the list if the requested version is missing.

After looking at the code a bit, it looks like that wouldn't actually be hard to implement since all of the necessary information is already available. It would also solve #104, which would be helpful if someone tries to install a version of Xcode that was released within a few hours of it being released.

I haven't tested it yet, but I'm thinking XcodeInstaller.downloadXcode would just need to be updated to something like this:

if self.xcodeList.shouldUpdate || self.xcodeList.availableXcodes.first(withVersion: version) == nil {

Do you have a preference on what approach is taken? Should xcodes list always update old lists and xcodes install only update if a version is missing? Or should both commands always update if the list is old, but with the addition that xcodes install also updates if the version is missing?

MattKiazyk commented 1 year ago

@rpendleton i think we should be smart as we can in that only update the list when we need to and not all the time.

Should xcodes list always update old lists and xcodes install only update if a version is missing?

Is a good assumption we can take

rpendleton commented 1 year ago

I've pushed a change that:

It ended up being a little bit more complicated than what I previously mentioned, because the list of available Xcodes was actually being checked in two different places depending on the data source. The first check was actually throwing an unavailableVersion error before it even had a chance to update the list, which is what caused the regression.

I cleaned that logic up a bit so that the list of available Xcodes is only checked in one place regardless of the data source, and only after the list of available Xcodes has had a chance to update.

(If you'd prefer, I can separate out the fix for that regression into a separate PR, but there would likely be conflicts between that PR and this one.)

rpendleton commented 1 year ago

For some reason, GitHub thought there was a conflict in the renamed main.swift/App.swift from #223 (even though a manual git merge didn't detect any actual conflicts), so I've merged main into this branch so that GitHub will stop saying there are conflicts.