Skip to content
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 Swift, ObjC examples code to showcase keygen and passphrase APIs #688

Merged
merged 8 commits into from
Jul 30, 2020

Conversation

vixentael
Copy link
Contributor

@vixentael vixentael commented Jul 29, 2020

During working on reproducing #687 I've noticed that Swift examples code doesn't show new SymmetricKeygen API and EncryptWithPassphrase API.

I've updated example projects to include usage of this API:

swift

  • iOS-Carthage
  • iOS-CocoaPods
  • macOS-Carthage

objc

  • iOS-Carthage
  • iOS-CocoaPods
  • macOS-Carthage

Checklist

- [ ] Change is covered by automated tests
- [ ] Benchmark results are attached (if applicable)
- [ ] Public API has proper documentation

  • The coding guidelines are followed
  • Changelog is updated (in case of notable or breaking changes)
  • Example projects and code samples are up-to-date (in case of API changes)

@vixentael vixentael requested a review from Lagovas as a code owner July 29, 2020 15:16
@@ -1,2 +1,2 @@
github "cossacklabs/themis" "0.13.0"
github "cossacklabs/themis" "gothemis/v0.13.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is truly funny

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-github "cossacklabs/themis" "0.13.0"
+github "cossacklabs/themis" "gothemis/v0.13.0"

No idea why this happens. It does not happen with "0.12.0", for example, which also has gothemis/v0.12.0 pointing to the same commit. Maybe Carthage does not like annotated signed tags?..

Well, I guess we can leave it like this since that's what users will be get in their projects. And the tags aren't going anywhere. But I'd like to have some fairies come and fix it ☁️

@vixentael vixentael requested a review from ilammy July 29, 2020 15:18
@vixentael vixentael added enhancement O-iOS 📱 Operating system: iOS W-SwiftThemis 🔶 Wrapper: SwiftThemis, Swift API labels Jul 29, 2020
Copy link
Collaborator

@ilammy ilammy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes seem okay, but I guess there is a bit more work to be done.

Also, what about Objective-C samples?

@@ -1,2 +1,2 @@
github "cossacklabs/themis" "0.13.0"
github "cossacklabs/themis" "gothemis/v0.13.0"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-github "cossacklabs/themis" "0.13.0"
+github "cossacklabs/themis" "gothemis/v0.13.0"

No idea why this happens. It does not happen with "0.12.0", for example, which also has gothemis/v0.12.0 pointing to the same commit. Maybe Carthage does not like annotated signed tags?..

Well, I guess we can leave it like this since that's what users will be get in their projects. And the tags aren't going anywhere. But I'd like to have some fairies come and fix it ☁️

Comment on lines 26 to 28
// generate key from pre-defined string
print("Using key from pre-defined string")
let keyFromString = self.generateMasterKey()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest to stop calling it generateMasterKey(), just put a constant here, and call it, like, fixedKey.

The TSGenerateSymmetricKey() is the preferred generator that we'd like to teach to the users.

SWIFT_VERSION = 4.2;
SWIFT_VERSION = 5.0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe other example projects need to be updated to use Swift 5.0 as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interestingly, Carthage example uses 5.0

Comment on lines 30 to 28
<label opaque="NO" userInteractionEnabled="NO" contentMode="left" horizontalHuggingPriority="251" verticalHuggingPriority="251" text="Copyright (c) 2015-2019 Cossack Labs. All rights reserved." textAlignment="center" lineBreakMode="tailTruncation" numberOfLines="0" baselineAdjustment="alignBaselines" adjustsFontSizeToFit="NO" translatesAutoresizingMaskIntoConstraints="NO" id="SDJ-TG-gQB">
<label opaque="NO" userInteractionEnabled="NO" contentMode="left" horizontalHuggingPriority="251" verticalHuggingPriority="251" text="Copyright (c) 2015-2020 Cossack Labs. All rights reserved." textAlignment="center" lineBreakMode="tailTruncation" numberOfLines="0" baselineAdjustment="alignBaselines" adjustsFontSizeToFit="NO" translatesAutoresizingMaskIntoConstraints="NO" id="SDJ-TG-gQB">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't the only project with an outdated copyright line.

$ git grep -o 'Copyright (c) .* All rights reserved.'
docs/examples/Themis-server/Obj-C/WorkingWithThemisServer/WorkingWithThemisServer/Base.lproj/LaunchScreen.storyboard:Copyright (c) 2015-2019 Cossack Labs. All rights reserved.
docs/examples/Themis-server/swift/SwiftThemisServerExample/SwiftThemisServerExample/Base.lproj/LaunchScreen.storyboard:Copyright (c) 2015-2019 Cossack Labs. All rights reserved.
docs/examples/objc/iOS-CocoaPods/ThemisTest/ThemisTest/Resources/Base.lproj/LaunchScreen.xib:Copyright (c) 2015-2019 Cossack Labs. All rights reserved." textAlignment="center" lineBreakMode="tailTruncation" baselineAdjustment="alignBaselines" minimumFontSize="9" translatesAutoresizingMaskIntoConstraints="NO" id="8ie-xW-0ye" userLabel="Copyright (c) 2015-2017 Cossack Labs. All rights reserved.
docs/examples/objc/iOS-CocoaPods/ThemisTest/ThemisTestTests/ThemisTestTests.m:Copyright (c) 2015 Cossack Labs. All rights reserved.
docs/examples/swift/iOS-CocoaPods/ThemisSwift/ThemisSwift/Resources/Base.lproj/LaunchScreen.storyboard:Copyright (c) 2015-2019 Cossack Labs. All rights reserved.

Also, how about leaving only the date when the company assumes copyright, implying that it's still actual in the present? That would save us the trouble of doing or forgetting to do pointless updates every year.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, makes sense

Copy link
Contributor Author

@vixentael vixentael Jul 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed copyright label at all from all projects, except Themis-server

@vixentael vixentael requested a review from ilammy July 29, 2020 22:28
@vixentael
Copy link
Contributor Author

Also, what about Objective-C samples?

done :)

@vixentael vixentael changed the title Update Swift examples code to showcase keygen and passphrase APIs Update Swift, ObjC examples code to showcase keygen and passphrase APIs Jul 29, 2020
@vixentael vixentael added W-ObjCThemis 🎨 Wrapper: ObjCThemis, Objective-C API M-Carthage Package manager: Carthage, Objective-C and Swift, iOS and macOS M-CocoaPods Package manager: CocoaPods, Objective-C and Swift, iOS and macOS O-macOS 💻 Operating system: macOS labels Jul 29, 2020
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Comment on lines +78 to +80
NSString * message = @"All your base are belong to us!";
NSString * context = @"For great justice";
NSError * themisError;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really the preferred code style ?

I mean , Apple docs definitely don ' t use punctuation like this . It also brings a lot of diff noise .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

¯_(ツ)_/¯
i aligned the code styles with that one that we've used in other examples.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, my fault for not adding code formatters. clang-format should do the trick for Objective-C and swift-format should work for Swift.

Well, I can agree to tolerate this but only because a) there is no formal code style, b) there are no formatters, c) a merit of unifying the style is displayed. Though, I can challenge the last point:

Project Code style
docs/examples/objc/iOS-Carthage 1️⃣ NSString *message
docs/examples/objc/iOS-CocoaPods 2️⃣ NSString * message
docs/examples/objc/macOS-Carthage 1️⃣ NSString *message
docs/examples/Themis-server/Obj-C 2️⃣ NSString * base64Body
src/wrappers/themis/Obj-C/objcthemis 1️⃣ NSMutableData *unwrappedMessage
tests/objcthemis/objthemis 2️⃣ NSString * sharedSecret

(So it's actually a tie. I won't count the lines.)

But FWIW, I'd personally keep whatever format is there in the files and use the same format for new code added there. The only acceptable reason for massive diff noise is some automated reformatting to the format which is going to be enforced later. If that's the thing, it should be in a separate PR anyway, not in the one which adds some new examples.

@vixentael vixentael merged commit 850f9ab into master Jul 30, 2020
@vixentael vixentael deleted the vxxtl/update-swift-examples branch July 30, 2020 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-Carthage Package manager: Carthage, Objective-C and Swift, iOS and macOS M-CocoaPods Package manager: CocoaPods, Objective-C and Swift, iOS and macOS O-iOS 📱 Operating system: iOS O-macOS 💻 Operating system: macOS W-ObjCThemis 🎨 Wrapper: ObjCThemis, Objective-C API W-SwiftThemis 🔶 Wrapper: SwiftThemis, Swift API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants