Skip to content
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.

Sign up for GitHub

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

Fix #2643: Added BUILD.bazel in accessibility package #2668

Closed
wants to merge 7 commits into from
Closed

Fix #2643: Added BUILD.bazel in accessibility package #2668

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
21dad44
Added BUILD.bazel in accessibility package
FareesHussain Feb 9, 2021
82083da
Resolving requested changes
FareesHussain Feb 10, 2021
c939ff5
Adding targets in app/BUILD.bazel
FareesHussain Feb 10, 2021
98bb093
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
FareesHussain Feb 15, 2021
8a8bead
Resolving requested changes
FareesHussain Feb 15, 2021
2575427
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
FareesHussain Feb 17, 2021
44aece9
Resolving Requested changes
FareesHussain Feb 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions app/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,6 +553,7 @@ kt_android_library(
"//third_party:androidx_databinding_databinding-common",
"//third_party:androidx_databinding_databinding-runtime",
"//utility",
"//utility/src/main/java/org/oppia/android/util/accessibility",
],
)

Expand Down Expand Up @@ -618,6 +619,7 @@ kt_android_library(
"//third_party:com_google_android_flexbox",
"//third_party:javax_annotation_javax_annotation-api_jar",
"//utility",
"//utility/src/main/java/org/oppia/android/util/accessibility",
],
)

Expand Down Expand Up @@ -683,6 +685,8 @@ TEST_DEPS = [
"//third_party:org_robolectric_annotations",
"//third_party:robolectric_android-all",
"//utility",
"//utility/src/main/java/org/oppia/android/util/accessibility:accessibility",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"//utility/src/main/java/org/oppia/android/util/accessibility:accessibility",
"//utility/src/main/java/org/oppia/android/util/accessibility",

Should be sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

"//utility/src/main/java/org/oppia/android/util/accessibility:testing",
]

# App module tests.
Expand Down
8 changes: 7 additions & 1 deletion utility/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@ load("//utility:utility_test.bzl", "utility_test")
# Library for general-purpose utilities.
kt_android_library(
name = "utility",
srcs = glob(["src/main/java/org/oppia/android/util/**/*.kt"]),
srcs = glob(
["src/main/java/org/oppia/android/util/**/*.kt"],
exclude = ["src/main/java/org/oppia/android/util/accessibility/**"],
),
custom_package = "org.oppia.android.util",
manifest = "src/main/AndroidManifest.xml",
resource_files = glob(["src/main/res/**/*.xml"]),
Expand Down Expand Up @@ -45,6 +48,9 @@ TEST_DEPS = [
"//third_party:org_jetbrains_kotlin_kotlin-test-junit",
"//third_party:org_mockito_mockito-core",
"//third_party:robolectric_android-all",
"//utility/src/main/java/org/oppia/android/util/accessibility:accessibility",
"//utility/src/main/java/org/oppia/android/util/accessibility:impl",
Copy link
Member

Choose a reason for hiding this comment

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

impl should be private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

"//utility/src/main/java/org/oppia/android/util/accessibility:testing",
]

utility_test(
Expand Down
32 changes: 32 additions & 0 deletions utility/src/main/java/org/oppia/android/util/accessibility/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
load("@io_bazel_rules_kotlin//kotlin:kotlin.bzl", "kt_android_library")
Copy link
Member

Choose a reason for hiding this comment

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

We should add a top-level documentation comment to this file explaining what sorts of files belong to it (e.g. General purposes utilities pertaining to the Android accessibility manager.).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

load("@rules_jvm_external//:defs.bzl", "artifact")

kt_android_library(
name = "accessibility",
srcs = ["CustomAccessibilityManager.kt"],
visibility = ["//visibility:public"],
)

kt_android_library(
name = "impl",
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, I think I missed the circular dependency that arises here when we try to make accessiblity depend on impl. There are a couple of different paths that we can take here, and I need to spend a bit more time thinking about this before proposing a specific solution. @fsharpasharp do you have a specific pattern that you think might work here?

Copy link
Contributor

@fsharpasharp fsharpasharp Feb 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

How about introducing a third library that exports the interface and impl? None of the existing libraries will depend on it.

Copy link
Member

Choose a reason for hiding this comment

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

That might unfortunately undermine the value of keeping impl separate (since it then exposes the public classes in impl which should stay private as part of impl). I think we might need to use a similar pattern where we export the modules of the package & have those depend on impl, but I'm not super keen on this pattern since it has its own drawbacks (such as requiring dependencies on all modules in central places like the android_binary or test targets).

Copy link
Member

Choose a reason for hiding this comment

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

Okay, let's try the module + impl approach and see how that looks. @FareesHussain I suggest changing this to be:

kt_android_library(
    name = "accessibility",
    visibility = ["//visibility:public"], 
    ... (source is interface only)
)

kt_android_library(
    name = "impl",
    ... (private visibility, source is accessibility module + prod implementation)
)

kt_android_library(
    name = "prod_module",
    visibility = ["//:oppia_binary_visibility"], 
    ... (source is new prod module -- see below; depends on :impl)
)

kt_android_library(
    name = "testing",
    visibility = ["//:oppia_testing_visibility"],
    testonly = True,
    ... (source is fake)
)

kt_android_library(
    name = "test_module",
    visibility = ["//:oppia_testing_visibility"],
    testonly = True,
    ... (source is new test module -- see below; depends on :testing)
)

You may also notice that there are both a test module & prod module. That's because I'm also suggesting that you make a functional change:

  • Rename AccessibilityModule to be AccessibilityManagerModule (to clarify it's just for providing the manager)
  • Introduce an AccessibilityProdModule that provides a binding of CustomAccessibilityManager to AndroidAccessibilityManager
  • Introduce an AccessibilityTestModule that provides a binding of CustomAccessibilityManager to FakeAccessibilityManager
  • Update all injections of AndroidAccessibilityModule to instead inject CustomAccessibilityManager so that it properly swaps out the accessibility version (you may be able to simplify some test bindings that are probably replacing the binding with the test version already)

Note also that the above requires introducing two new visibility groups: oppia_binary_visibility & oppia_testing_visibility to simplify visibility management. The former should work as-is, but the latter may not work until we split up the tests (in which case we should introduce the visibility such that it applies to all subpackages, and is later restricted to tests).

The nice thing about this package is that it actually includes all of the pieces we want to demonstrate in the pattern:

  • A public API
  • A production implementation that is kept private
  • A production module that should use the private implementation & be incorporated in the top-level binary (or in tests that want to use the production module)
  • A testing module that can optionally be used in tests
  • A fake testing library

If you're unsure of all this, let me know. I'll introduce a new PR that replaces this one in that case since it may be easier for me to demonstrate the new pattern more explicitly that way.

Copy link
Member

Choose a reason for hiding this comment

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

@fsharpasharp as well--what do you think of the above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BenHenning I'll follow up with your PR regarding this pattern.

srcs = [
"AccessibilityModule.kt",
"AndroidAccessibilityManager.kt",
],
deps = [
":accessibility",
artifact("com.google.dagger:dagger:2.27"),
Copy link
Member

Choose a reason for hiding this comment

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

As discussed in chat, prefer using the third party wrappers instead of artifact since they offer a bunch of benefits (see #2663 for specifics).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

artifact("javax.inject:javax.inject:1"),
],
)

kt_android_library(
name = "testing",
testonly = True,
srcs = ["FakeAccessibilityManager.kt"],
visibility = ["//visibility:public"],
deps = [
":accessibility",
artifact("javax.inject:javax.inject:1"),
],
)