Skip to content

Conversation

@jkmassel
Copy link
Contributor

@jkmassel jkmassel commented Nov 6, 2025

Description

Cleans up the WordPressComLanguageDatabase object to:

  1. Validate at launch-time that the Languages.json file is correct (and avoid repeatedly parsing it)
  2. Remove a bunch of Obj-C stuff.
  3. Delete custom parsing code in favour of Codable.

Testing instructions

The existing tests pass, so this should be low-risk.

@jkmassel jkmassel requested review from crazytonyli and kean November 6, 2025 21:18
@dangermattic
Copy link
Collaborator

dangermattic commented Nov 6, 2025

4 Warnings
⚠️ Modules/Package.swift was changed without updating its corresponding Package.resolved.

If the change includes adding, removing, or editing a dependency please resolve the Swift packages as appropriate to your project setup (e.g. in Xcode or by running swift package resolve).

If the change to the Package.swift did not modify dependencies, ignoring this warning should be safe, but we recommend double checking and running the package resolution just in case.
.

⚠️ View files have been modified, but no screenshot or video is included in the pull request. Consider adding some for clarity.
⚠️ This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️ This PR is assigned to the milestone 26.5. This milestone is due in less than 4 days.
Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Nov 6, 2025

App Icon📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress
ConfigurationRelease-Alpha
Build Number29687
VersionPR #24973
Bundle IDorg.wordpress.alpha
Commit3e5717c
Installation URL0sv6397l9rrc8
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Nov 6, 2025

App Icon📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack
ConfigurationRelease-Alpha
Build Number29687
VersionPR #24973
Bundle IDcom.jetpack.alpha
Commit3e5717c
Installation URL1j8fvt3ekqso8
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@jkmassel jkmassel added this to the 26.5 milestone Nov 6, 2025
@jkmassel jkmassel force-pushed the update/WordPressComLanguageDatabase branch from c884d4e to f458626 Compare November 6, 2025 21:59
@kean
Copy link
Contributor

kean commented Nov 6, 2025

Hey, looks like this PR has changes from #24972. Does it need a different target branch?

@jkmassel jkmassel force-pushed the update/WordPressComLanguageDatabase branch from f458626 to c3c3b97 Compare November 7, 2025 01:31
@jkmassel
Copy link
Contributor Author

jkmassel commented Nov 7, 2025

Hey, looks like this PR has changes from #24972. Does it need a different target branch?

Should be sorted!

// Parse the json file
let path = Bundle.wordPressSharedBundle.path(forResource: "Languages", ofType: "json")
let data = try! Data(contentsOf: URL(fileURLWithPath: path!))
let bundle = try! JSONDecoder().decode(WPComLanguageBundle.self, from: data)
Copy link
Contributor

Choose a reason for hiding this comment

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

The languages are hard-coded anyway. I wonder if it'd be simpler to just hard-code with instances, instead of reading and parsing a JSON file:

struct {
  let popular: [Language] = [
    Language(...)
  ]
  ...
}

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 agree 😄

See Automattic/wordpress-rs#1011

/// This helper class allows us to map WordPress.com LanguageID's into human readable language strings.
///
public class WordPressComLanguageDatabase: NSObject {
public struct WordPressComLanguageDatabase {
Copy link
Contributor

Choose a reason for hiding this comment

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

I was confused by this type for a bit, because there is another one with the same name in WordPressKit: https://github.com/wordpress-mobile/WordPress-iOS/blob/fd99f270ff1c1c8015128567e45441c0ee018de4/Modules/Sources/WordPressKit/WordPressComLanguageDatabase.swift

Maybe we should delete one of these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice thanks! Done now.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 7, 2025

@jkmassel jkmassel added this pull request to the merge queue Nov 7, 2025
Merged via the queue into trunk with commit 46c9fa7 Nov 7, 2025
30 of 32 checks passed
@jkmassel jkmassel deleted the update/WordPressComLanguageDatabase branch November 7, 2025 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants