-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Move some functions in post models to Swift #25003
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
base: trunk
Are you sure you want to change the base?
Conversation
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 29850 | |
| Version | PR #25003 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 0c20429 | |
| Installation URL | 66hgu4dfl0rgg |
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 29850 | |
| Version | PR #25003 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 0c20429 | |
| Installation URL | 2pn8kl8bef90o |
128254a to
0c20429
Compare
|
| } else if self.shouldPublishImmediately() { | ||
| return NSLocalizedString("Publish Immediately", comment: "A short phrase indicating a post is due to be immedately published.") | ||
| } | ||
| return self.dateCreated?.toMediumString() |
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.
It's super rough that dateCreated can be nil – do we know of any scenario where it should be?
|
|
||
| var authorDisplayName: String { | ||
| settings.author?.displayName ?? post.authorNameForDisplay() | ||
| settings.author?.displayName ?? post.author?.makePlainText() ?? "" |
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.
Same issue here – any reason the author should be nullable?
| return cell | ||
| } | ||
|
|
||
| if post.isCross() { |
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.
What a weird auto-ObjC translation – I had to check that isCross() and isCrossPost were the same in Xcode 🙃
| public func avatarURLForDisplay() -> URL? { | ||
| authorAvatarURL.flatMap(URL.init(string:)) | ||
| } | ||
| public func sourceAttributionStyle() -> SourceAttributionStyle { |
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.
Because this takes no params, I wonder if it should be a var but that's a nit
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.
Same note for the other func translations
jkmassel
left a comment
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.
Was super easy to review commit-by-commit, thanks!





Description
No feature changes, just moving to Swift. It'd be easier to review commit by commit to compare the original Objective-C code and the new Swift code.
Note
I'll merge this PR after #24999.
Testing instructions