-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Migrate project to Kotlin #289
Conversation
...on and more minor changes
@vmadalin wow! This is a massive and very well thought out contribution. First of all thank you for taking the time to do this. In general it's good to file an issue before sending a PR like this so you don't have to do any unnecessary work though.
Without looking too deeply here are some things I like:
- The logo is awesome! Let's definitely use that.
- The code reorganization is a good idea. Although we shouldn't move any classes which were part of the publicly documented API before, organization isn't worth breaking people's code.
- I like moving the sample to Kotlin, shows off more modern practices.
Here are some things that scare me though:
- Moving to Kotlin is definitely a breaking change and this would be
4.0.0. I am worried that doing this is mostly valuable to us and only causes churn for our developers. Is there anything about the API that gets much nicer in Kotlin? Should we consider just doing aneasypermissions-ktxinstead? - If we are going to do a breaking change, I would like to first fix as many of the open issues as we can in the Java version and release a
3.0.1so that people can get a stable version of the library without adopting Kotlin.
cc @SUPERCILEX who always knows what the Android community is up to. Would you say that it's "in fashion" for Java libraries to move to Kotlin or is that considered a hostile move?
@samtstern Thanks for your answer. I'm absolutely agree to focus effort trying to fix first the open issues, and upload a fix with these changes. Making more robust the current version. It's important also to migrate these fixes in the kotlin version.
Regarding the version, and repo. In my opinion it's not bad to think to create a new repository easypermissions-ktx or a new branch but the only problem what I see is to maintenance, both projects separately. Although while decide the best option maybe we should to create a specific branch called kotlin or similar and merge there all PR related and giving people the opportunity to contribute with next refactor version of the library
- The logo is awesome! Let's definitely use that.
- The code reorganization is a good idea. Although we shouldn't move any classes which were part of the publicly documented API before, organization isn't worth breaking people's code.
- I like moving the sample to Kotlin, shows off more modern practices.
👆 I agree with all of this!
Some thoughts:
- Moving to Kotlin shouldn't be a breaking change. In theory, we can make the APIs exactly the same as before to keep supporting our Java users.
- The main benefits of Kotlin are NPE avoidance, faster development cycles, and more pleasant/modern language to use that makes life easier.
- I would argue that we see none of those benefits. EasyPermissions is essentially a feature-complete library and thus in maintenance mode. That means we aren't actively developing it so a more enjoyable language that makes development faster is irrelevant. As for the NPEs we seen, they're mostly due to OEM bugs which means it would still be broken for Kotlin.
- Kotlin presents a 1MB+ size increase for devs not using it previously. For something as fundamental as permissions, I feel like we'd want to minimize bloat and keep everybody happy (including Java people).
- Major libraries that have switched to Kotlin are doing it because they can offer radically superior APIs (think Jetpack Compose, coroutines, operator + extension functions, property delegation, etc), but even that's sometimes unnecessary since you can usually just add extension functions in a companion lib to do the same thing (think KTX).
- @samtstern There's also some confidential information I shared with you.
TL;DR: I would wholeheartedly support a KTX version of EasyPermissions, but I'm a little skeptical of having the whole library switched to Kotlin. Until 70% of the top 100K apps with updates in the last 6 months use Kotlin, I think libraries should generally stick to Java. (Or if JetPack releases a core library in Kotlin.)
Thanks @SUPERCILEX, those are all great points and I agree.
@vmadalin I still think your work is very valuable so would it be possible to reduce this PR to just the changes to app code but not touch the core library yet? I am not comfortable with actually shipping any Kotlin code to our developers but I am happy to adopt Kotlin within the repository for our own productivity!
I would also be open to moving the tests to Kotlin (as you've already done) but I'm not sure if there is any way to do that and still safely ship a Java library so I think we have to hold on on that for now.
And then if you want to pursue easypermissions-ktx in the future let's open a new issue where we can discuss what the API would look like before we implement anything.
@SUPERCILEX I'm absolutely agree with all points that you comment. But obviously depends a lot of project factors. For example at Userzoom, we decided to migrated the SDK to Kotlin recently but the distribution of it is in .jar file, for keeping the compatibility with different frameworks/platforms like react-native, phonegap, xamarin and flutter. The tendencies in a long time is to migrate all projects but this is a big task and require time, and for the moment the tendency is to have a hybrid project (specially for big projects).
@samtstern I consider that isn't a good practice to have tests in kotlin and the library in java that create a lot of confusion on development process, specially if you use TDD or BDD. Maybe the only part what we could migrate is the sample, keeping or not the both samples. The project structure is also valuable base to start to discuss how it would be better to organise the project, facilitating its maintenance.
However I see as good to pursue easypermissions-ktx to start discuss the new API thinking in simplify the integration, for example it's a good start point compare another notification permission like: https://github.com/permissions-dispatcher/PermissionsDispatcher with a very interest integration method.
MadRatSRP
commented
Nov 6, 2020
Is there any progress in moving to Kotlin and publishing 4.X version?
@MadRatSRP no progress, I think the conversation above stands. I'd be open to converting the sample app and (maybe) the tests to Kotlin. I'd also be open to a KTX library. Converting the core library to Kotlin has no additional user benefit so I don't think it's a good investment.
MadRatSRP
commented
Nov 7, 2020
@MadRatSRP no progress, I think the conversation above stands. I'd be open to converting the sample app and (maybe) the tests to Kotlin. I'd also be open to a KTX library. Converting the core library to Kotlin has no additional user benefit so I don't think it's a good investment.
Actually, it has a lof of the additional user benefit. As you know, current Android development flows around Kotlin (including Kotlin Native) and Flutter so if somebody will convert the whole library into Kotlin, that will make it much more popular. And your code will help someone to learn something new about Kotlin and other things
@MadRatSRP that's true, but all of that can be accomplished via an easypermissions-ktx library, there's no need to convert the core library to Kotlin.
@vmadalin hey it looks like you're back! Are you still planning to pursue this PR?
@vmadalin hey it looks like you're back! Are you still planning to pursue this PR?
Hi @samtstern, I was involved on other things this year and I decided to dedicate some time to move forward the work I did. My intention was to continue this under https://github.com/VMadalin/easypermissions-ktx, of course, keeping google licence.
I'm not sure if is possible to add under googlesample/easypermissions-ktx keeping the consistency with the java repo?
@vmadalin that sounds like a good plan. We can't add your repo under googlesamples (that's for Google-owned code only) but if you want I am happy to link to https://github.com/VMadalin/easypermissions-ktx from the README of this repo.
That sounds amazing @samtstern, I let you know when I'm done ;)
@vmadalin I'm going to close this PR since at this point we both agree we're not going to merge all of this. When you're ready please send a new PR to add a link to the README, or just send me an email (samstern at google dot com) and I'll do it!
Uh oh!
There was an error while loading. Please reload this page.
logo
Description
Migrate all entire project to
Kotlin1.3.50, but keeping the integration concept. These are some of the things that have been done:ktlint,detekt,spotlessfor static analysis code adding them toTravisRationale/Settingsdialogs displaying anAlertDialogLowAPiPermissionsfor avoid throw exception to integrator on their codeOverview
Screenshot 2019年09月06日 at 11 49 22
Screenshot 2019年09月06日 at 17 10 43
Next Steps
Obviously with these changes, we only migrate the project to kotlin with little changes or simplifications. It's a good point to continue in this line and try to simplify the integration of the library and not only also adding more coverage tests, code documentation, etc..