Ensure generated Kotlin code follow Kotlin naming convention #62
Conversation
| /** | ||
| * Whether the user has the people space enabled | ||
| */ | ||
| val WebMetaSpacePeopleEnabled: Boolean? = null, | ||
| val webMetaSpacePeopleEnabled: Boolean? = null, |
There was a problem hiding this comment.
This are the problems reported by Detekt, a static analysis tool.
There was a problem hiding this comment.
this looks like an issue in the source file https://github.com/matrix-org/matrix-analytics-events/blob/main/schemas/UserProperties.json
I'm not sure if posthog will handle casing as unique properties~
There was a problem hiding this comment.
That's why I did not change the value we send to Posthog but just the "internal" Kotlin code
There was a problem hiding this comment.
I think I’d agree with Adam that this should be fixed in the schema to maintain consistency.
There was a problem hiding this comment.
OK, did not want to break the existing schema. @novocaine OK for you (as it will impact the Web code)?
There was a problem hiding this comment.
The issue is we can't adjust the Typescript generator with tweaks to maintain existing data without a lot of work given it is external to this project https://www.npmjs.com/package/json2ts
There was a problem hiding this comment.
I do not understand why it's preventing this PR to be merged. This PR does not change the plan and do not change the typescript code.
There was a problem hiding this comment.
I never said anything here is preventing this PR being merged.
I'm arguing with
in this instance I believe the schema itself is inconsistent and would lean towards it being changed to avoid future confusion
There was a problem hiding this comment.
OK, sorry 🤗 . CC @ouchadam ^
There was a problem hiding this comment.
properties should be camelCase, I think that's fairly clear from examples in the README and in the code.
Web has been used as an enum prefix elsewhere which is why it was an uppercase W there, but I don't see why it should be so here, where we are describing a property. It should start with web.
In terms of how disruptive this is to fix, I would be pretty surprised if posthog was case-sensitive..?
UserProperties fix is also in matrix-org/matrix-analytics-events#62
|
codewise the fix LGTM, there's still a pending question of if this change is needed and sets a precedent for allowing future inconsistent property naming perhaps we can merge this and create a separate issue to provide a github action to linter to enforce consistency in the json schemas |
UserProperties fix is also in matrix-org/matrix-analytics-events#62
Constructor parameters cannot start by an upper case letter.
This PR fixes that, hopefully without any side effect on the sent analytics, and on the other generated code.