Skip to content

Conversation

danielceinos
Copy link
Contributor

@danielceinos danielceinos commented Sep 24, 2020

PR's key points

Updated dependencies:

  • Kotlin from 1.3.31 to 1.4.10
  • Build tools from 3.4.1 to 4.0.1
  • App compact from 1.0.2 to 1.2.0
  • Google play core from 1.6.1 to 1.8.0 Breaking changes

Also added plugin to version lib from git tags

Definition of Done

  • Tests pass
  • Works with Proguard
  • There is no outcommented or debug code left (if there is a good reason, use a // FIXME: comment or // TODO: comment and create a follow-up issue)

@danielceinos danielceinos force-pushed the task/update-dependencies branch from d6da86e to bfe358e Compare September 24, 2020 16:24
Co-authored-by: Adrián García <adriangarcia.lopez@gmail.com>
Copy link
Contributor

@adriangl adriangl left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @danielceinos!
I'll take a deeper look into it on the weekend, but for now just add the suggestion I did and fix the conflicts with the base branch

@danielceinos
Copy link
Contributor Author

@adriangl how we could test that the new version still works? using internal testing works good?

@adriangl
Copy link
Contributor

@adriangl how we could test that the new version still works? using internal testing works good?

Looks like it, according to the docs.
I honestly don't remember if that option was available when I first implemented the lib (I'd say no), so I can't confirm if it works as expected

minSdkVersion project.ext.minSdkVersion
targetSdkVersion project.ext.targetSdkVersion
versionCode 1
minSdkVersion 21
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think about doesnt couple example app with the lib? @adriangl

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean the minSdkVersion, or the app's versionCode?

If it's the former, you could update the ones in the root's build.gradle, which currently point to API 29 and use them here as well as in app.

If it's the latter, I'd keep app's versionCode to 1 for a very simple reason: so we always have the lowest version code available to test the in-app updates there :P

If you meant anything else, I didn't get it xD

Copy link
Contributor Author

@danielceinos danielceinos Sep 28, 2020

Choose a reason for hiding this comment

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

I was talking only about lib gradle file. Its a bit awkward need to look at the example app to get what minSdk version the library has, do you understand why I mean?

I'm OK with has versionCode to 1 at app's gradle.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the delay. Yeah, we can leave the versions decoupled ;)

minSdkVersion project.ext.minSdkVersion
targetSdkVersion project.ext.targetSdkVersion
versionCode 1
minSdkVersion 21
Copy link
Contributor

Choose a reason for hiding this comment

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

You mean the minSdkVersion, or the app's versionCode?

If it's the former, you could update the ones in the root's build.gradle, which currently point to API 29 and use them here as well as in app.

If it's the latter, I'd keep app's versionCode to 1 for a very simple reason: so we always have the lowest version code available to test the in-app updates there :P

If you meant anything else, I didn't get it xD

@danielceinos danielceinos requested a review from adriangl October 3, 2020 10:13
@adriangl adriangl self-assigned this Oct 7, 2020
@adriangl adriangl merged commit 0ceaf87 into hyperdevs-team:master Oct 7, 2020
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.

2 participants