Skip to content

Add ExPlat implementation to :experimentation module - #114

Merged
RenanLukas merged 13 commits into
trunkfrom
issue/112-extract-explat
Mar 9, 2022
Merged

Add ExPlat implementation to :experimentation module#114
RenanLukas merged 13 commits into
trunkfrom
issue/112-extract-explat

Conversation

@RenanLukas

@RenanLukas RenanLukas commented Mar 4, 2022

Copy link
Copy Markdown
Contributor

Resolves #112

Copies ExPlat implementation from WordPress-Android so it can be used by other projects that depend on Automattic-Tracks-Android.

private val coroutineScope: CoroutineScope,
private val isDebug: Boolean
) {
private val platform = Platform.WORDPRESS_ANDROID

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe this should be declared by client

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Great catch! Thank you

import com.automattic.android.experimentation.Experiment

@Suppress("unused")
class ExampleExperiment(exPlat: ExPlat) : Experiment(name = "example_experiment", exPlat = exPlat)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If it's not used anywhere, maybe we can remove it to not cause confusion?

import java.util.Date

@ExperimentalCoroutinesApi
class ExPlatTest {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, love the coverage!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you for implementing it @renanferrari 😁

Comment thread experimentation/build.gradle Outdated
Comment on lines +33 to +34
lintOptions {
baseline file("lint-baseline.xml")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As it's a new module, the baseline is not needed

Comment thread experimentation/build.gradle Outdated
versionCode 1
versionName "1.0"

testInstrumentationRunner "androidx.test.runner.AndroidJUnitRunner"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As this module has no instrumentation tests, let's remove this to not add anything not needed.

…d.gradle, inject Platform field in ExPlat, update ExPlat unit tests
@RenanLukas

Copy link
Copy Markdown
Contributor Author

Thank you for the initial review @wzieba ! I've addressed your comments.

Any ideas on why CI is complaining about :AutomatticTracks?

@wzieba

wzieba commented Mar 7, 2022

Copy link
Copy Markdown
Member

@RenanLukas I'm investigating this issue, for this moment:

  • lint started to fail on Cursor#getColumnIndex
  • the quick fix is a change: Cursor#getColumnIndex -> Cursor#getColumnIndexOrThrow
  • this happens in different projects as well (e.g. wordpress-mobile/WordPress-FluxC-Android@979125f)
  • the weird thing is that it happens also if you checkout some older version of the library (e.g. commit a957ae9, when it was passing few months ago when it was building on CI). The same thing is reported by different people. I'm wondering what is the case, because version of Android Lint is associated with Android Gradle Plugin so without any code changes it shouldn't break 🤔 .

Either way this issue is not directly associated with your code changes. I believe it'd be awesome to fix it in a separate PR but ideally it'd be awesome to find a root cause - EventTable shouldn't be touched by any means, nothing really changed in implementation.

Comment thread experimentation/build.gradle Outdated
}

android {
compileSdkVersion 32

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@RenanLukas generally, the core issue is that compileSdkVersion of this module, influenced which version of lint/lint rules will be used. So lowering it down to 30 will resolve the issue. I think the best will be to bump compileSdkVersion of both modules in a seperate PR.

Here is branch on which I was testing, see that build is passing on CI: https://github.com/Automattic/Automattic-Tracks-Android/compare/issue/112-extract-explat...issue/112-testing?expand=1

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you very much @wzieba 👍

@RenanLukas
RenanLukas marked this pull request as ready for review March 7, 2022 15:40
@RenanLukas
RenanLukas requested a review from renanferrari March 7, 2022 15:41

@renanferrari renanferrari left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for handling this!

LGTM 🚢

@RenanLukas
RenanLukas requested a review from wzieba March 8, 2022 01:18

@wzieba wzieba left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me!

@RenanLukas
RenanLukas merged commit 4d4a662 into trunk Mar 9, 2022
@RenanLukas
RenanLukas deleted the issue/112-extract-explat branch March 9, 2022 17:17
@RenanLukas RenanLukas mentioned this pull request Mar 10, 2022
11 tasks
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.

Extract ExPlat from WordPress-Android to Automattic-Tracks-Android

3 participants