-
Notifications
You must be signed in to change notification settings - Fork 139
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
Update the cached list of available Xcodes if it's more than 24 hours old #226
Update the cached list of available Xcodes if it's more than 24 hours old #226
Conversation
In my first assumption, I mentioned that an alternative for 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 if self.xcodeList.shouldUpdate || self.xcodeList.availableXcodes.first(withVersion: version) == nil { Do you have a preference on what approach is taken? Should |
@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.
Is a good assumption we can take |
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.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update - it works great and is a fantastic addition!
For some reason, GitHub thought there was a conflict in the renamed main.swift/App.swift from #223 (even though a manual |
Since `xcodes` v1.1.0 this is no longer required, because it's automatically done if more than 24 hours has passed since the last `xcodes list`. - XcodesOrg/xcodes#226 This reverts commit e2f54e5.
This fixes #202, although I made a few assumptions that I'm open to discussion about.
Specifically:
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.)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.
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.