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

Attempt at code simplification #923

Merged
merged 5 commits into from
Oct 4, 2023
Merged

Conversation

oxtoacart
Copy link

No description provided.

@oxtoacart oxtoacart requested a review from jigar-f October 3, 2023 21:59
@@ -58,103 +59,74 @@ const CURRENT_TERMS_VERSION = 1
const IS_PLAY_VERSION = "playVersion"
const SET_SELECTED_TAB = "/selectedTab"

// All method names
// this expose to client IOS & Andorid
const SESSION_MODEL_METHOD_INIT_MODEL = "initSesssionModel"
Copy link
Contributor

Choose a reason for hiding this comment

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

@oxtoacart Any reason why we are not using const here? I think we were using the same const on the IOS end to have method name consistency

Copy link
Author

Choose a reason for hiding this comment

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

I'm in the process of getting rid of those calls from iOS. When the Swift code calls Go, it's cleaner just to expose an actual method rather than trying to call through InvokeMethod. Once that's done, there's no need for these to be constants since the only remaining place they're used is Dart, which doesn't have access to our Go constants anyway. And I think it makes the Go code more readable to just have the strings right inside of the switch statement.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for explaining. I got it.

@oxtoacart oxtoacart marked this pull request as ready for review October 4, 2023 11:37
@oxtoacart
Copy link
Author

I'm going to go ahead and merge this.

@oxtoacart oxtoacart merged commit 7a90c8d into ios-migrate Oct 4, 2023
1 of 2 checks passed
@oxtoacart oxtoacart deleted the ox/ios-migrate-suggestions branch October 4, 2023 19:20
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