search

TelemetryDeck / docs

Public Documentation for TelemetryDeck
https://telemetrydeck.com/docs
5 stars 8 forks source link

Add new Grand Rename article explaining why and how it's done #85

Closed Jeehut closed 6 months ago

Jeehut commented 7 months ago

@winsmith These are my renaming suggestions. If approved, I can create pull requests on all mobile SDKs that apply them. I would also go through other docs and adjust references accordingly once approved, I only adjusted one article so far.

TODOs that still need to be discussed or are out of my scope are:

winsmith commented 7 months ago

Hey @kkostov, @pichfl and @timidak, I'd love your feedback on these changes regarding renaming and cleaning up parameters the SDK is sending.

Jeehut commented 7 months ago

@winsmith I just addressed all comments and also updated my original comment here, removing the TODOs we already clarified and adding one new entry. I also updated https://github.com/TelemetryDeck/SwiftClient/pull/149 accordingly with the suggested changes. Now to your main comment points:

operatingSystem and platform – I think there is some history here

Does that mean we should keep them "as-is"? I can understand that, I have no strong feelings here. Just wanted to bring it up that it confused me while we were talking renaming stuff.

Regarding these namespaced entries, we also have TelemetryDeck.Presets.Onboarding.step. Should Presets also be a namespace, or do you want to rename that to something else?

I think we should stick to 3 parts, so TelemetryDeck.Presets.Onboarding.step is too long. I will make sure to adjust the PRs where I introduce the presets after this has been merged (or maybe before). But I think it's a separate discussion. I will add any new group names to the renaming document on those PRs.

Can you please note which entries should not be strings? If we're sending floats or ints (which is very advisable in some cases) , I'll need to tell the server in advance.

For these two new fields sending a number could help:

  1. TelemetryDeck.Device.screenResolutionWidth
  2. TelemetryDeck.Device.screenResolutionHeight

Let me know if you change them, I have implemented them as Strings for now. There's one more we might send an integer for, but I'm not sure if it really makes a difference:

  1. TelemetryDeck.AppInfo.buildNumber: This is an integer on all mobile platforms.
Jeehut commented 7 months ago

I want to point out a few changes I made in the latest commits:

  1. I renamed AppContext to RunContext to further clarify its difference to AppInfo to avoid confusion.
  2. I introduced the new prefix UserPreference for region and preferredLanguage, the latter I could then shorten to just language.

I would like to also point out one renaming decision I had made earlier in this PR which is easy to miss: I had changed the order of words in the old majorSystemVersion and majorMinorSystemVersion in the new names to have the same prefix system (for the very same reasons we have prefixes at all: better grouping). They are now systemVersion, systemMajorVersion, and systemMajorMinorVersion.

Jeehut commented 6 months ago

I just updated more documents alongside creating https://github.com/TelemetryDeck/SwiftClient/pull/153 which applies the changes to the SwiftSDK. Please note that before we ship this to production, I should be doing a full test with the whole onboarding process from creating a new app to setting up everything following the docs to make sure nothing is broken.

I did not touch the Objective-C code by the way, neither in the docs, nor in the SDK. I consider Obj-C as "deprecated" in itself, so renaming anything there seems to be waste of time to me. Let me know if you think otherwise.

winsmith commented 6 months ago

I just updated more documents alongside creating TelemetryDeck/SwiftClient#153 which applies the changes to the SwiftSDK. Please note that before we ship this to production, I should be doing a full test with the whole onboarding process from creating a new app to setting up everything following the docs to make sure nothing is broken.

Fantastic work, and yes please do a full test run.

I did not touch the Objective-C code by the way, neither in the docs, nor in the SDK. I consider Obj-C as "deprecated" in itself, so renaming anything there seems to be waste of time to me. Let me know if you think otherwise.

Sensible, I agree with this decision.

Jeehut commented 6 months ago

@winsmith I just did a full setup run to test if the Swift setup guide looks good and everything is right with the Swift SDK. I found a couple of bugs and fixed them. From my point of view, the Swift part of the grand rename is now ready. So this and https://github.com/TelemetryDeck/SwiftClient/pull/153 could be both merged. Don’t forget to rename the SwiftClient to SwiftSDK after the merge.

I personally don’t think there’s need to wait for the Android/Flutter SDK before merging this, because:

  1. I have not touched any of their setup guides (only the Swift one), so they can be updated at a later time.
  2. As long as we don’t market the “Grand Rename” article, no one will be aware of it.
  3. As long as there’s no new 2.0.0 release on GitHub of the SwiftSDK, no one will notice the change (unless they rely on the main branch, which should be rare).

But feel free to take your time to think about it and give me feedback.

I’ll be focusing on the Purchases & Errors presets feature next. I will rebase the branches on top of the grand-rename branches and make sure make all adjustments needed and to include a built-in solution for the presets in the SwiftSDK so setting up the presets is even less typing work.