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

Support enum with whitespaces #105

Merged
merged 2 commits into from
May 29, 2024
Merged

Conversation

BillCarsonFr
Copy link
Member

@BillCarsonFr BillCarsonFr commented May 28, 2024

When using white spaces in oneOf enum types the generated kotlin and swift code was not valid (white space in the property name).

The main changes are in kotlin.py and swift.py, typescript generation was ok with whitespaces.

The change will be invisible for users of the library.

Kotlin

For kotlin, a new field rawValue is added in the generated enum and is used in the getProperties method instead of the enum autogenerated name.

Before:

    enum class VerificationState {
          NotVerified,
          ...
          
          override fun getProperties(): Map<String, Any>? {
               ...
               put("verificationState", verificationState.name)
               ...
          }

After

    enum class VerificationState(val rawValue: String) {
          NotVerified("NotVerified"),
          ...
          
          override fun getProperties(): Map<String, Any>? {
               ...
               put("verificationState", verificationState.rawValue)
               ...
          }

Swift

The enum were alreasy : String just now explicitely adding the rawvalue

Before:

        public enum VerificationState: String {
            ...
            case NotVerified

Now:

        public enum VerificationState: String {
            ...
            case NotVerified = "NotVerified"

@BillCarsonFr BillCarsonFr marked this pull request as ready for review May 28, 2024 14:28
@BillCarsonFr BillCarsonFr requested a review from a team as a code owner May 28, 2024 14:28
@BillCarsonFr BillCarsonFr requested review from bmarty and richvdh May 28, 2024 14:28
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. (Can't tell for generated Swift code)

Comment on lines 17 to 35
"appPlatform": {
"description": "Used by web to identify the platform (Web Platform/Electron Platform).",
"type": "string"
},

"platformCodeName": {
"description": "Used as a discriminant to breakdown usage per client.",
"type": "string",
"oneOf": [
{"const": "Web", "description": "Element Web platform code."},
{"const": "Desktop", "description": "Element Desktop platform code."},
{"const": "EI", "description": "Element iOS platform code."},
{"const": "EXI", "description": "Element-X iOS platform code."},
{"const": "EA", "description": "Element Android platform code."},
{"const": "EXA", "description": "Element-X Android platform code."},
{"const": "Other", "description": "Other Platform code."}
{"const": "Web Platform", "description": "Element Web platform."},
{"const": "Electron Platform", "description": "Element Desktop platform."},
{"const": "EI", "description": "Element iOS platform."},
{"const": "EXI", "description": "Element-X iOS platform."},
{"const": "EA", "description": "Element Android platform."},
{"const": "EXA", "description": "Element-X Android platform."},
{"const": "Other", "description": "Other Platform."}
]
}
},
Copy link
Member

Choose a reason for hiding this comment

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

I suggest making changes to the properties definitions in a separate PR to the changes to the script, to make it easier for reviewers and future readers to see what is changing.

Copy link
Collaborator

@bmarty bmarty May 29, 2024

Choose a reason for hiding this comment

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

Yes, you're right, but we do not have the bandwidth to ensure 150% quality on all our projects... Also adding a const with a space in this PR ensure that the change on the script generation works as expected.

I was tempted to tell @BillCarsonFr to better split their commits, for instance I do not like that commit on the script change also contains the generated files. It should be distinct commits, to easily filter out commit with generated code.
But again let's not spend to much time on this.
A better commit history would have been:

  • Update the script to support const with spaces
  • Run the script
  • Update appPlatform names
  • Run the script

I guess this can be a better recipe for next time.

Copy link
Member Author

Choose a reason for hiding this comment

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

I force pushed this branch to only contain the script changes with proper history
Script Change 39eeb1c

Run script to modify generated files 641e66f

And there is a new PR to update appPlatform and remove platfromCodeName #106

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you're right, but we do not have the bandwidth to ensure 150% quality on all our projects.

I think this is a bit like saying "we don't have time to write tests". You can save time in the short term, but in the longer term, breaking up changes into smaller steps that are easier to verify is an important way of avoiding bugs.

Copy link
Member

Choose a reason for hiding this comment

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

(Thanks @BillCarsonFr for breaking this up - I think it's much clearer now)

@BillCarsonFr BillCarsonFr merged commit 009ca1d into main May 29, 2024
3 checks passed
@BillCarsonFr BillCarsonFr deleted the valere/support_enum_whitespace branch May 29, 2024 09:56
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