-
-
Notifications
You must be signed in to change notification settings - Fork 18
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 support to Android using Kotlin code generation #53
Conversation
Thank you soooo much for this PR, @husseinala ! I'm very excited to review it, test it and provide feedback! I'll only be able to take a look at this by the end of the week though, probably over the weekend! I know there are many people interested in this, so I'll tag them here so they can follow the evolution of this new feature/PR as it happens and can also provide valuable feedback and maybe even help us test a bit in diverse projects/work environments 😊 @JARMourato @humblerookie @Shafichariri @houmie @mrea1 @yuukiw00w @AF-cgi @mrea1 @marceltex 🙇 Thanks a lot again @husseinala, and congrats on your first contribution here! ❤️ PS: Great PR description, thanks for the thorough explanation! 🤩 |
Thank you @rogerluan! Looking forward to the feedback from everyone 😊 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its missing the README adjustments 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note
Partial review for now!
I left some minor comments so far, as I haven't reviewed the core logic of the PR. Some things I noticed (also piggy backing on some of the comments that were already left above by other reviewers):
- We should be updating the README as well to reflect all the changes (e.g. remove the "coming soon" section, adding proper usage documentation for the new kotlin features, perhaps add some more instructions on how to adequate your project for kotlin generation, etc).
- Kotlin unit tests are super valuable as they guarantee the actual encoding/decoding is working as expected, and that it doesn't regress in the future :) could you add those?
- Do the generated kotlin files pass a standard kotlin linter out-of-the-box? One with a "widely adopted" style guide 😇
- Last but not least, could you also implement an e2e test to be run in CI via Rakefile, like we're doing with Swift? These references will be helpful:
Looking forward to continue reviewing this PR when I have some extra time! It's looking promising! 🙌
Thanks @JARMourato, @marceltex & @rogerluan for taking the time to go through the PR 🙏🏼 , will work on addressing all the raised issues and push all the necessary changes, including making sure all the generated code passes Kotlin's official codeing conventions |
Watermelon AI SummaryAI Summary deactivated by husseinala GitHub PRs
arkana is an open repo and Watermelon will serve it for free. |
Hi @rogerluan, sorry for the delay, I've been a bit busy the last two weeks, but I finally got around to making the requested changes which address the following points:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I finished reviewing this PR and addressing all the nitpicks I had in mind 🤓
Solid PR @husseinala you did an awesome work here! Thank you so much!
There are just a couple things left that I asked you to review, whenever you have some time 🙇 and I just left some misc questions too 😊 . Other than that, could you also merge the latest master into this branch, resolving the conflicts? They should be simple (just minor text changes) 🙇
Looking forward to getting this merged! 🚀🚀
# Conflicts: # template.yml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've merged latest master into this branch, this is now good to go!! 🚀
Thanks once again for your contribution @husseinala! ❤️
Yaay 🎉 thank you for taking the time to review and clean up the PR @rogerluan. Really glad I was able to contribute to this project! ❤️ |
Resolves #1
Description
Where I work, we've been using Arkana on our iOS projects and we're big fans of the project, so would like to adopt it for our Android projects too, which is why we thought it would be helpful to kick off the process of adding Kotlin support.
This PR adds a
KotlinCodeGenerator
and modifies the execution entry point to determine which generator to run based on the specified--lang
argument flag.The following changes have also been made:
Arkana Arguments
--lang
flag used to specify the generator to use for e.g.kotlin
. Defaults toswift
.Arkana YAML config
kotlin_package_name
option which is used by the Kotlin generator to determine the package name of the generated module. Defaults tocom.arkanakeys
.kotlin_sources_path
option which is used by the Kotlin generator to determine the path for generated Kotlin classes. Defaults to src/main/kotlin.should_generate_gradle_build_file
option which is useful if yourresult_path
is a path of an existing module and you would like to avoid generating a newbuild.gradle
file.Generated module structure
Generated sources
{namespace}Environment.kt
Contains interface declaration for the environment-specific keys.
{namespace}.kt
Contains the Keys object: