Add version check and prompt for self update #313
Conversation
|
I'm thinking about possibly refactoring this in some way to make things coherent, maybe all as a method under swiftly/Sources/Swiftly/Swiftly.swift Line 83 in 2cedc32 |
Sources/Swiftly/Install.swift
Outdated
| mutating func run(_ ctx: SwiftlyCoreContext) async throws { | ||
| try validateSwiftly(ctx) | ||
|
|
||
| let swiftlyRelease = try await ctx.httpClient.getCurrentSwiftlyRelease() |
There was a problem hiding this comment.
question: can we put this code in validateSwiftly() as a kind of validation check? I think that it might make sense to have it there.
There was a problem hiding this comment.
I see that one problem is with the defer block. Perhaps the validate function can return a function that should be deferred by the caller?
There was a problem hiding this comment.
That's a good idea, this would also make all commands that does validateSwiftly(ctx) do version checks. Which includes self-update , for now I have discarded the returned func for self-update as we don't need checks before the command that actually updates itself.
Sources/Swiftly/Install.swift
Outdated
| let shouldUpdateSwiftly = try swiftlyRelease.swiftlyVersion > SwiftlyCore.version | ||
| defer { | ||
| if shouldUpdateSwiftly { | ||
| ctx.print("A new release of swiftly is available") |
There was a problem hiding this comment.
suggestion: Print these messages to stderr so that it doesn't interfere with other tools that might be parsing stdout. It also has the benefit of being able to quieted at the shell if a user wants using redirects like this: swiftly install 2>/dev/null
There was a problem hiding this comment.
Is there a predefined way of doing prints to stderr? Currently I'm using fileHandler to route to stderr, I see that print() depends on the context and would it be better modifying outputHandler in the context to be stderr and then change it back to being stdout?
There was a problem hiding this comment.
I think that it's ok in this context to print directly to stderr outside of the ctx.print(). The context is there for the purpose of mocks during testing, and I don't think that this feature warrants testing of what it prints to stderr.
In terms of how to print to stderr, I think that these FileHandle API's in Foundation could work for this:
https://developer.apple.com/documentation/foundation/filehandle/standarderror
https://developer.apple.com/documentation/foundation/filehandle/write(contentsof:)
There was a problem hiding this comment.
Oh nice, that's exactly what I used for printing out to stderr, I'll mark this pr for review then, thanks!
|
It looks like the big URL -> FilePath created conflicts on this PR. |
|
Hi @cmcgee1024 I just merged main into this branch, lmk if you need anything else! |
Sources/Swiftly/Swiftly.swift
Outdated
| if shouldUpdateSwiftly { | ||
| let updateMessage = """ | ||
| ----------------------------- | ||
| A new release of swiftly is available |
There was a problem hiding this comment.
nitpick: This sentence needs a period at the end.
|
@swift-ci test macOS |
Sources/Swiftly/Swiftly.swift
Outdated
| // Verify that the configuration exists and can be loaded | ||
| _ = try await Config.load(ctx) | ||
|
|
||
| let swiftlyRelease = try await ctx.httpClient.getCurrentSwiftlyRelease() |
There was a problem hiding this comment.
issue(blocking): This will throw an error if there's some kind of an HTTP connection problem fetching the current swiftly release. That shouldn't abort the principal operation, such as use/install/etc.
suggestion: Change this to try? await so that the release is optional. Only if the release can be found and it is larger than the current version would trigger the message.
There was a problem hiding this comment.
Sounds good, done.
|
@louisunlimited I suspect that the tests are failing because mocked http executor is producing fatal errors due to this attempt to make a network request. The idea is to keep the vast majority of tests from making real network requests so that they can run quickly and are immune to changes on swift.org that make them brittle. Perhaps this could produce a mocked swiftly version instead, based on the current version in SwiftlyVersion? |
|
@louisunlimited you can fix the formatting error by running |
Yeah I was going through tests locally and I see that by wrapping tests inside something like try await SwiftlyTests.withMockedSwiftlyVersion(latestSwiftlyVersion: SwiftlyVersion(major: SwiftlyCore.version.major, minor: 0, patch: 0)) {
...
}would fix it. I could just wrap this around all the tests (or the helper func they call), but was thinking if there are better ways. Any suggestions? |
I think that's pretty consistent with what's being done in other cases in the test suite, so that seems fine to me. |
|
@cmcgee1024 Just updated the tests! Lmk what you think, and also if it's necessary to add a "shouldPromptVersionUpate" test for each of the commands. End up with using Traits instead of wrapping them because there seems to be some ordering issues with how ctx is being passed around. All test are passing locally. |
|
@swift-ci test macOS |
1 similar comment
|
@swift-ci test macOS |
|
Thanks for your contribution @louisunlimited |
2 participants
Manually added checks at the start of each command (apart from
self-updateandinit) to check for version mismatches, if found, it will try to prompt the user with:It also tries to prompt the user at the very last stage of the command, ensuring the update prompt will always show at the end to avoid being buried.