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

Confusing naming of new navigation APIs #162

Closed
Jeehut opened this issue Jun 28, 2024 · 0 comments · Fixed by #164
Closed

Confusing naming of new navigation APIs #162

Jeehut opened this issue Jun 28, 2024 · 0 comments · Fixed by #164
Assignees

Comments

@Jeehut
Copy link
Contributor

Jeehut commented Jun 28, 2024

I just noticed that I failed to give feedback on #161 about the confusing naming of the newly suggested functions (to my defense, I was still in jet lag and not fully back to work yet). A call named TelemetryDeck.navigate(from: "Source.path", to: "Destination.path") reads like an API which executes navigation because it's using the imperative form. Users might think it helps with SwiftUI navigation or similar, which isn't the case.

The preset helpers all use a past form to indicate that "something happened", not "something will happen thanks to this call". The error function is named errorOccurred and the purchases one is called purchaseCompleted. Similarly, I would suggest we rename this new API right away (before too many people use it) to be navigated, navigationPathChanged, navigationChanged, or at least navigation if you don't like the others (which isn't technically past, but at least it doesn't sound like it executes navigation). I personally prefer navigationPathChanged because that would be consistent with the signal name Navigation.pathChanged which is the exact same logic I applied for the other helpers which were named Error.occurred and Purchase.completed respectively.

I've also noticed you placed the helpers right inside the TelemetryDeck.swift file whereas the other helpers are in respective extensions named TelemetryDeck+Errors.swift and TelemetryDeck+Purchases.swift. I could adjust the name with a fix-it provided (at least for a short time) and I also could move the code to an extra file for consistency. I think the main TelemetryDeck.swift file should really focus on the main functionality, which is mainly the signal function.

@winsmith Please let me know what you think about the rename.

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 a pull request may close this issue.

1 participant