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

SPM Prep – Rewrite RemoteBlogSettings encoding and decoding in Swift #780

Open
wants to merge 10 commits into
base: trunk
Choose a base branch
from

Conversation

mokagio
Copy link
Contributor

@mokagio mokagio commented Apr 7, 2024

Description

The Objective-C BlogServiceRemoteREST object references the Swift types RemoteBlog and RemoteBlogSettings.

This Objective-C to Swift dependency is incompatible with the separation we require for the SPM setup.

The first step to address this is to move the custom encoding and decoding logic implemented within BlogServiceRemoteREST to Swift.

I did this by first writing tests for the existing implementation and then moving it to Swift using those tests as a guideline.

Testing Details

See new tests in CI.


  • Please check here if your pull request includes additional test coverage. — N.A.
  • I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary. — N.A.

@mokagio mokagio force-pushed the mokagio/remote-blog-settings-swift branch from e028eca to d2c4249 Compare April 7, 2024 22:55
@dangermattic
Copy link
Collaborator

1 Warning
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

@mokagio mokagio force-pushed the mokagio/remote-blog-settings-swift branch from d2c4249 to 0ace60c Compare April 7, 2024 23:03
@mokagio mokagio changed the title Rewrite RemoteBlogSettings encoding and decoding in Swift SPM Prep – Rewrite RemoteBlogSettings encoding and decoding in Swift Apr 7, 2024
@mokagio mokagio requested a review from crazytonyli April 7, 2024 23:04
@mokagio mokagio enabled auto-merge April 7, 2024 23:05
languageID = rawSettings.number(forKey: CodingKeys.languageID.rawValue)
iconMediaID = rawSettings[CodingKeys.iconMediaID.rawValue] as? NSNumber
// The value here can be a String, so we need a custom conversion
gmtOffset = rawSettings.number(forKey: CodingKeys.gmtOffset.rawValue)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I worry too much, but do you think we should use number(forKey:) to parse all properties instead of just these keys, because that's what the original Objective-C uses?.

We happen to know these weird cases, simply because we have one test json file for it. There is no indication in the original Objective-C code that these keys need special treatment. And, may be there are other keys needs a special "string" to "number" parsing, which is not detected by that one single test json file?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, the string(forKey:) parsing in the original Objective-C code can parse a number to a string, which we don't do here. So, maybe we should use string(forKey:) and number(forKey:) instead of type casting?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants