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

Make it easier to check for appLanguage, preferredLanguage, and region + grand rename #153

Merged
merged 16 commits into from
May 23, 2024

Conversation

Jeehut
Copy link
Contributor

@Jeehut Jeehut commented May 3, 2024

This is a follow-up on #149 with the branch renamed to grand-rename and the decisions from TelemetryDeck/docs#85 applied. This includes the changes from #149:

In Swift, Locale.current represents the language and region the app is currently using. When an app is not localized to say German, for example, the identifier will not return de for the language even it's the users preferred language.

To make it easier for TelemetryDeck users to use a data-driven approach in deciding which languages to localize for next, it is important to add the preferred language of the user though, even if the app doesn't support it. It can also be useful to see just the app language or just the region, without them being clutched together in locale as in en_DE.

This PR therefore introduces region (the regional part of locale), appLanguage (the language part of locale), and the new preferredLanguage which is the field that is most useful to make the localization expansion decision.

In addition, I have renamed TelemetryManager.send("A", with: ["a": "b"]) to TelemetryDeck.signal("A", parameters: ["a": "b"]) as well as TelemetryManager.initialize to TelemetryDeck.initialize as per TelemetryDeck/docs#85 (comment).

@Jeehut
Copy link
Contributor Author

Jeehut commented May 3, 2024

Please note that I found a for parameter and also a floatValue parameter on the send function. I renamed for to customUserID to be consistent with the configurations defaultUserIdentifier (which could be renamed to defaultUserID in the future). The floatValue didn't change. But I found that the order was like this:

  1. Signal Name (required)
  2. Custom User ID
  3. Float Value
  4. Parameters

It's more common to place parameters used more often at the front though and ones that are usually skipped to fall back to the default values at the end. So I changed the order as follows (basically switched 2 and 4):

  1. Signal Name (required)
  2. Parameters
  3. Float Value
  4. Custom User ID

To still provide fix-its for most cases, I added a copy of the function which only takes the Signal Name and Parameters and made sure it's favored over the function with more parameters if only 2 are provided. Otherwise, Xcode is unable to apply fix-its when the order of parameters change, but I think fix-its can be really useful to save time in migration.

@Jeehut
Copy link
Contributor Author

Jeehut commented May 18, 2024

I just updated this with a few bug fixes from my full setup testing. Biggest change is the addition of a new TelemetryDeck target which was needed so import TelemetryDeck works. I basically added an empty target which just imports the stuff from TelemetryClient so existing users with already-written code don’t get affected and everything continues to work.

Copy link
Contributor

@winsmith winsmith left a comment

Choose a reason for hiding this comment

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

Fantastic work, I appreciate the docs a lot!

@winsmith
Copy link
Contributor

Seems like the linter is failing but from the error message it's not something introduced in this PR

@Jeehut
Copy link
Contributor Author

Jeehut commented May 22, 2024

@winsmith I think it's related to the upgrade to Swift 5.9 to properly support the visionOS platform in the Package manifest. That requires Xcode 15, and the CI has Xcode 14.

@Jeehut Jeehut force-pushed the feature/grand-rename branch 2 times, most recently from 01e1850 to 23b957f Compare May 23, 2024 19:24
@Jeehut Jeehut force-pushed the feature/grand-rename branch from 23b957f to 3a45e61 Compare May 23, 2024 19:25
@Jeehut
Copy link
Contributor Author

Jeehut commented May 23, 2024

@winsmith I just adjusted the CI configuration and fixed all tests and more. Here's all I did:

  • Upgrade to Xcode 15 (from 14)
  • Replace the previous macOS-based SwiftLint step without the --strict option with an Ubuntu-based one that does have the --strict option, so warnings lead to fails of the CI (what's the point of the lint step if warnings are ignored?)
  • I fixed a bunch of warnings SwiftLint gave me, which were mostly internal naming issues (so API shouldn't be affected)
  • I changed the scheme of the test commands to TelemetryClient-Package as the auto-generated TelemetryClient scheme does not have a test target (anymore)
  • I added visionOS as a test destination to ensure we keep support for Apple Vision Pro

Please take another look.

@Jeehut Jeehut requested a review from winsmith May 23, 2024 19:31
@winsmith winsmith merged commit 0c5dcdd into main May 23, 2024
6 checks passed
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.

2 participants