Modernize app example #231

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.

Sign up for GitHub

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

Jump to bottom

Merged

fabianfett merged 5 commits into awslabs:main from stevapple:update-app

Dec 6, 2021

Contributor

stevapple commented Sep 25, 2021 •

edited

Loading

Update LocalDebugging example to reflect the latest SwiftUI practice.

Motivation:

Since we already required Swift 5.5 with Concurrency (Xcode 13), it's natural to update the app part to reflect latest SwiftUI changes, which makes the code neater and more expressive.

Modifications:

Update LocalDebugging/MyApp to use SwiftUI lifecycle;
Update LocalDebugging/MyApp to reflect SwiftUI best practice;
Update LocalDebugging/Shared to use Swift 5.5.

Result:

A better demo app.

swift-server-bot commented Sep 25, 2021

Can one of the admins verify this patch?

6 similar comments

swift-server-bot commented Sep 25, 2021

Can one of the admins verify this patch?

swift-server-bot commented Sep 25, 2021

Can one of the admins verify this patch?

swift-server-bot commented Sep 25, 2021

Can one of the admins verify this patch?

swift-server-bot commented Sep 25, 2021

Can one of the admins verify this patch?

swift-server-bot commented Sep 25, 2021

Can one of the admins verify this patch?

swift-server-bot commented Sep 25, 2021

Can one of the admins verify this patch?

stevapple force-pushed the update-app branch 2 times, most recently from 8a1e613 to 0ebb9dd Compare

September 25, 2021 03:09

tomerd added the 🔨 semver/patch label

Contributor

tomerd commented Sep 25, 2021

thank @stevapple !

lets get #228 in first

Contributor

tomerd commented Sep 25, 2021

@swift-server-bot test this please

tomerd reviewed

View reviewed changes

...calDebugging/MyApp/MyApp.xcodeproj/project.xcworkspace/xcshareddata/IDEWorkspaceChecks.plist Outdated Show resolved Hide resolved

tomerd reviewed

View reviewed changes

Examples/LocalDebugging/MyApp/MyApp.xcodeproj/project.xcworkspace/contents.xcworkspacedata Outdated Show resolved Hide resolved

stevapple force-pushed the update-app branch from 0ebb9dd to 61a8a2d Compare

September 25, 2021 08:47

stevapple added 2 commits

September 25, 2021 16:49


          Use a more modern app example

b96b66b


          Update Swift version

b16d0c8

stevapple force-pushed the update-app branch from 61a8a2d to dc491d5 Compare

September 25, 2021 08:50

Contributor Author

stevapple commented Sep 25, 2021

I've rebased the branch against #228, so there should be no pain to merge.

Contributor

tomerd commented Sep 26, 2021

@swift-server-bot test this please

stevapple force-pushed the update-app branch from 57ba0f5 to 6bf66f6 Compare

September 26, 2021 23:46


          Minor Fixes

2b48c13

stevapple force-pushed the update-app branch from 6bf66f6 to 2b48c13 Compare

September 26, 2021 23:57

Contributor

tomerd commented Sep 27, 2021

@swift-server-bot test this please

fabianfett reviewed

View reviewed changes

Examples/LocalDebugging/MyApp/MyApp/ContentView.swift Outdated

    
                          guard let httpResponse = response as? HTTPURLResponse else {

                              throw CommunicationError(reason: "invalid response, expected HTTPURLResponse")

                              return "invalid response, expected HTTPURLResponse"

Contributor

fabianfett Sep 27, 2021

I'm personally not a big fan of removing the CommunicationError here, since it hides what actually happens. We need to have some do {} catch {} somewhere else. This method should be async throws -> String.

Contributor Author

stevapple Sep 28, 2021

Uh yes.

I changed it into () async -> String because we need to ensure the label is changed in every path. throws could be preserved here and it doesn't affect.

Contributor

fabianfett Sep 28, 2021 •

edited

Loading

I would prefer something like this:

func register() await -> String {
    do {
        return try await self.runNetworkRegister(name: self.name, password: self.password)
    } catch as CommunicationError {
        return error.reason
    } catch {
        return error.localizedDescription
    }
}

Contributor Author

stevapple Sep 28, 2021 •

edited

Loading

I would rather suggest we catch in the button action? The register() API is expected not to confuse an actual response and a thrown Error. runNetworkRegister(name:password:) looks redundant here.

Contributor Author

stevapple Sep 28, 2021 •

edited

Loading

Conditional catch can also be eliminated here by implementing LocalizedError.


          Update ContentView.swift

3c8345f

stevapple force-pushed the update-app branch from f39d76f to 3c8345f Compare

September 28, 2021 10:36

fabianfett approved these changes

View reviewed changes

Contributor

fabianfett left a comment

Let's wait for @tomerd approval!

Contributor

fabianfett commented Oct 2, 2021

tomerd approved these changes

View reviewed changes


          Merge branch 'main' into update-app

Contributor

tomerd commented Oct 2, 2021

@swift-server-bot test this please

Contributor Author

stevapple commented Oct 29, 2021

Xcode 13.2 and Swift Playgrounds 4 supports a new type of package-based app project (.swiftpm), and a preview of it can be seen here.

I’m wondering if such change is too aggressive as most of us still use .xcodeproj in production. @fabianfett @tomerd

Contributor Author

stevapple commented Nov 14, 2021

ping @tomerd @fabianfett

fabianfett merged commit 39b34a1 into awslabs:main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🔨 semver/patch