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

Add a new platform code super property #104

Merged
merged 1 commit into from
May 27, 2024

Conversation

BillCarsonFr
Copy link
Member

Fixes https://github.com/element-hq/crypto-internal/issues/316

Unfortunately I cannot just use the existing app platform, because it uses values with white space (Web Platform). The script we use to generate swift/kotlin enum will generate invalid code for such values.
We are using some custom python script to convert, not sure if worth finding how to fix them

@BillCarsonFr BillCarsonFr requested a review from a team as a code owner May 27, 2024 13:25
@BillCarsonFr BillCarsonFr requested review from langleyd and bmarty May 27, 2024 13:25
Copy link
Collaborator

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@BillCarsonFr BillCarsonFr merged commit 53b2e89 into main May 27, 2024
3 checks passed
@BillCarsonFr BillCarsonFr deleted the valere/more_super_platform branch May 27, 2024 14:02
@@ -38,6 +38,10 @@ data class SuperProperties(
* Version of the crypto backend.
*/
val cryptoSDKVersion: String? = null,
/**
* Used as a discriminant to breakdown usage per client.
Copy link
Member

Choose a reason for hiding this comment

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

break down

@richvdh
Copy link
Member

richvdh commented May 28, 2024

I have to say that having two properties that do the same thing seems like a very bad idea. Can't we just fix the script?

At the very least, the comments should say how the new property differs from the old one, and which we should be using in future.

@BillCarsonFr
Copy link
Member Author

I have to say that having two properties that do the same thing seems like a very bad idea. Can't we just fix the script?

At the very least, the comments should say how the new property differs from the old one, and which we should be using in future.

Yes fixing the script would be better. But given that the existing appPlatform is only used by web, I thought I might as well deprecate it and avoid the effort to change all the existing code generation script. On web it's also used in RS and maybe it was a matomo thing previously.

@richvdh
Copy link
Member

richvdh commented May 28, 2024

Well, could you at least add a comment to appPlatform to say that it is deprecated?

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