Skip to content
This repository was archived by the owner on Sep 15, 2025. It is now read-only.

Conversation

@crazytonyli
Copy link
Contributor

Description

Similar to #556, this PR is part of re-implementing Objective-C in Swift.

  • Please check here if your pull request includes additional test coverage.
  • I have considered updating the version in the .podspec file.

@crazytonyli crazytonyli requested a review from mokagio December 8, 2022 08:30
Copy link
Contributor

@mokagio mokagio left a comment

Choose a reason for hiding this comment

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

Looking good. I tried to read the code, although I don't trust my eyes to be able to reliably pick up translation issues... 🤔

But:

  • I trust your work
  • We have unit tests
  • If something breaks in this process it'll be a good chance to add more tests

import Foundation
import NSObject_SafeExpectations

/// This class encapsulates all of the *remote* Blog properties
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know what a remote blog is? And what's the difference with a "non-remote (?)" blog?

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 think "remote" indicates it's an HTTP API model, which is defined by the WordPress API schema (hence the "remote" name?). A "non-remote" blog (or just Blog) means it's a custom model type defined by the app, which may have a different structure than the "remote" one.

@@ -1,6 +1,4 @@
#import "AccountServiceRemoteREST.h"
#import "RemoteBlog.h"
#import "RemoteBlogOptionsHelper.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Chasing down these two imports, I noticed their footprint is minimal in this implementation. As far as I could see, only this method:

- (NSArray *)remoteBlogsFromJSONArray:(NSArray *)jsonBlogs
{
NSArray *blogs = jsonBlogs;
return [blogs wp_map:^id(NSDictionary *jsonBlog) {
return [[RemoteBlog alloc] initWithJSONDictionary:jsonBlog];
}];
return blogs;
}

Might it be useful to re-write it in Swift in the context of this PR?

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 intentionally started with the easy ones to translate. This AccountServiceRemoteREST falls into my harder basket, and will be dealt with after I have finished with the easy ones 🥲

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure.

Just to be clear, I was suggesting converting only that method, but I can see how adding a dedicated extension just for it would be wasteful given we'd remove it once the whole class is converted soon enough.

public var quotaSpaceUsed: NSNumber?

/// Parses details from a JSON dictionary, as returned by the WordPress.com REST API.
public init(JSONDictionary json: NSDictionary) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sure you thought of this, too, but at some point it'd be good to make this and the other types currently using JSONDictionary Decodable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. Once I've finished the Swift translation, I'll have a look at the NSDictionary usage across all the models. I'll probably start with specializing the argument type first, to be [String: Any] for example. I'm not sure about Decodable, as it's a more drastic change to the JSON parsing code. But I'll for sure considering adopting Decodable when I get to it. 👍

Base automatically changed from update-nsobject-expecations to trunk December 8, 2022 20:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants