Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings
This repository was archived by the owner on Oct 3, 2024. It is now read-only.

Migrate project to Kotlin #289

Closed
vmadalin wants to merge 23 commits into googlesamples:master from vmadalin:vm/migrate-to-kotlin

Conversation

@vmadalin
Copy link

@vmadalin vmadalin commented Sep 6, 2019
edited
Loading

logo

Description

Migrate all entire project to Kotlin 1.3.50, but keeping the integration concept. These are some of the things that have been done:

  • Migrate entire sample to kotlin
  • Make the library more modular by structure better the files / code
  • Add a project logo, make more easy to identify it (It is just a proposal 😄 , please share you point of view)
  • Add severals tools like ktlint, detekt, spotless for static analysis code adding them to Travis
  • Migrate all the tests to kotlin, for guaranty the coverage for new code
  • Modify readme specify the minim API version, and increase the version to 3.1.0 (please review this part if you consider to make major change)
  • Simplify logic about Rationale/Settings dialogs displaying an AlertDialog
  • Eradicate LowAPiPermissions for avoid throw exception to integrator on their code

Overview

  • Architecture:

Screenshot 2019年09月06日 at 11 49 22

  • Coverage:

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..

GoldenSoju reacted with thumbs up emoji seventhmoon reacted with thumbs down emoji thatfiredev reacted with confused emoji
Copy link
Contributor

@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 an easypermissions-ktx instead?
  • 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.1 so that people can get a stable version of the library without adopting Kotlin.
thatfiredev reacted with thumbs up emoji

Copy link
Contributor

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?

Copy link
Author

vmadalin commented Sep 9, 2019

@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

Copy link
Contributor

  • 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.)

Copy link
Contributor

samtstern commented Sep 12, 2019
edited
Loading

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.

Copy link
Author

@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.

Copy link

Is there any progress in moving to Kotlin and publishing 4.X version?

Copy link
Contributor

samtstern commented Nov 6, 2020
edited
Loading

@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.

SUPERCILEX reacted with thumbs up emoji

Copy link

@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

Copy link
Contributor

@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.

Copy link
Contributor

@vmadalin hey it looks like you're back! Are you still planning to pursue this PR?

Copy link
Author

@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?

Copy link
Contributor

@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.

vmadalin reacted with thumbs up emoji

Copy link
Author

That sounds amazing @samtstern, I let you know when I'm done ;)

Copy link
Contributor

@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!

vmadalin reacted with thumbs up emoji

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /