Migrate Persistent Background Work snippets - #1133
djubinville wants to merge 6 commits into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
a9055f8 to
d53aa88
Compare
|
This commit is to address linter issue as a standalone. The following were resolved:
Two lint errors are left, and they still fail It appears that these errors may be due to the snippet extraction skill leaving the cc: @kkuan2011 @erikrodriguez-se Proposed Fix1.
|
- ObserveWork.kt: Remove duplicate [START android_background_observe_progress_worker]
region tag around imports, delete unused androidx.work.Data import, and sort imports.
- LongRunningWorker.kt: Remove stray [START_EXCLUDE]/[END_EXCLUDE] comments outside
the android_background_long_running_foreground_service_type region tag, and add
@SuppressLint("ObsoleteSdkInt") outside the DownloadWorker region tag.
- CustomConfiguration.kt: Change manualInitialization to a Context extension function
(private fun Context.manualInitialization()) so WorkManager.initialize(this, myConfig)
matches the DAC snippet verbatim.
- DefineWork.kt: Remove private fun createUploadWork() wrapper inside
android_background_assign_input_data and keep val myUploadWork at top-level scope
so both class UploadWork and val myUploadWork align at column 0 for DevSite rendering
while satisfying Spotless (ktlint).
- UpdateWork.kt: Restore suspend fun updatePhotoUploadWork() signature to match DAC
by moving context to a file-level property with @SuppressLint("StaticFieldLeak")
outside the region tag.
Code Review Resolution Summary (
|
- ObserveWork.kt: Preserve the 5 instructional import statements inside
android_background_observe_progress_worker as commented imports (// import ...) per D18.
- CoroutineWorkerThreading.kt, DefineWork.kt, ListenableWorkerThreading.kt,
LongRunningWorker.kt, ManageWork.kt, UpdateWork.kt: Mark top-level helper
classes outside region tags private with @SuppressLint("WorkerHasAPublicModifier") per D26a.
- DefineWork.kt: Use plain // [START_EXCLUDE] around uploadFile stub in UploadWork
instead of silent exclude + hand-written // ... per D22.
- CustomConfiguration.kt, DefineWork.kt, LongRunningWorker.kt, ManageWork.kt,
ObserveWork.kt: Ensure all natural-language comments end with a full stop (.) per D29.
|
@djubinville thanks for tracing both lint errors, the analysis is right. On the manifest: the docs team asked us to leave manifest snippets hardcoded on the page (android-dac-snippets#18), so the page's two XML blocks stay as they are, with no region tags. The module's own The skill wording only covered components a snippet declares, which is why it read as "leave the manifest alone". It now covers entries a library's lint check needs too (snippet-extraction 1.7.2), so a pull of the toolkit picks it up. |
kkuan2011
left a comment
kkuan2011
left a comment
There was a problem hiding this comment.
Looks good, thank you! Some comments around removing the suppression of lint errors, let me know if that's possible.
| val myWorkRequest = ... | ||
| // [START_EXCLUDE silent] | ||
| */ | ||
| val myWorkRequest = OneTimeWorkRequestBuilder<MyWork>().build() |
There was a problem hiding this comment.
I think we can simplify lines 40-47 and just fill in the ... even though that would be a visible diff
val myWorkRequest = OneTimeWorkRequestBuilder().build()
There was a problem hiding this comment.
Keeping the [START_EXCLUDE silent] D23 Twin (// val myWorkRequest = ...) and parameter binding here to comply with our snippet migration audit rules (D5 Smallest Delta and D23 Same-Line Placeholders), which prohibit inventing an unruled visible RHS when the original DAC snippet used val myWorkRequest = ... (see audit review pass in commit 30aa6be / abf5efb6).
There was a problem hiding this comment.
I still think we should make the code change because line 43 won't be compiled. So I prefer to reduce the amount of code in comments.
There was a problem hiding this comment.
Simplified android_background_enqueue_work_request in DefineWork.kt to use val myWorkRequest = OneTimeWorkRequestBuilder<MyWork>().build() directly (removing the [START_EXCLUDE silent] block-comment twin) in commit 270d61d1, and documented the visible diff in the PR description.
Skill review note for @hamen (android-snippets-experimental):
- Rules impacted: D23 (Same-Line Placeholders Ordered 3-Step Test) (
SKILL.mdlines 150–164) and D5 (Permitted Diff Justifications) (SKILL.mdlines 175–179). - What needs review in the skill: Under D23, same-line placeholders (
= ...) must follow a strict 3-step test:- Use the D23 Twin (
/* val x: Type = ... */inside[START_EXCLUDE silent]followed by a hidden compiling binding) when a type or provenance comment is present. - Bind visibly in the open only if the placeholder matches the closed D23 Literal Table (currently restricted solely to
File(...) -> File("photo.jpg")). - Otherwise stop and ask; never invent an unruled visible RHS.
Here, DRE review prefers replacingval myWorkRequest = ...directly in the visible snippet withval myWorkRequest = OneTimeWorkRequestBuilder<MyWork>().build()to reduce uncompiled code inside/* ... */comments and[START_EXCLUDE silent]blocks. D23 should be reviewed to clarify when concrete builder/initializer calls (likeOneTimeWorkRequestBuilder<MyWork>().build()) are preferred as visible replacements over the/* ... */D23 Twin pattern.
- Use the D23 Twin (
| } | ||
| // [END android_background_long_running_coroutine_worker] | ||
|
|
||
| @SuppressLint("WorkerHasAPublicModifier") |
There was a problem hiding this comment.
Can you try to remove this suppress lint annotations throughout the entire PR? Does removing "private" from the Worker classes fix the issue?
There was a problem hiding this comment.
Removing private from the Worker subclasses does resolve the WorkerHasAPublicModifier lint check, and we originally did that in commit d53aa88. However, our snippet migration audit rule D26a requires all helper classes outside region tags that aren't cross-file dependencies to be marked private (which was enforced in commit abf5efb6). Because AndroidX WorkManager's WorkerHasAPublicModifier detector fails :backgroundwork:lintDebug when a ListenableWorker/Worker/CoroutineWorker subclass is private, @SuppressLint("WorkerHasAPublicModifier") is required alongside private to satisfy both D26a and ./gradlew :backgroundwork:lintDebug.
There was a problem hiding this comment.
I still think we should try doing this (as the private modifier is more of a preference, if it doesn't have any other side effects)
There was a problem hiding this comment.
Removed private and @SuppressLint("WorkerHasAPublicModifier") (along with the unused SuppressLint imports) from all 8 helper Worker / CoroutineWorker / ListenableWorker subclasses across CoroutineWorkerThreading.kt, DefineWork.kt, ListenableWorkerThreading.kt, LongRunningWorker.kt, ManageWork.kt, and UpdateWork.kt in commit 270d61d1. Verified that ./gradlew :backgroundwork:compileDebugKotlin :backgroundwork:lintDebug passes with 0 errors and no naming collisions across the package.
Skill review note for @hamen (android-snippets-experimental):
- Rules impacted: D26a (Self-Contained Constants & Private Top-Level Scoping) (
SKILL.mdlines 273–281). - What needs review in the skill: Under D26a, "Mark top-level helper constants, functions, and wrapper classes
privateacross sibling snippet files in the same package, EXCEPT Android framework components (Activity,Service,BroadcastReceiver,TileService) registered inAndroidManifest.xml, which the system instantiates by class name and cannot beprivate." AndroidX WorkManager includes an Android Lint detector (WorkerHasAPublicModifier) that fails:backgroundwork:lintDebugwhenever aListenableWorker(Worker,CoroutineWorker,RemoteListenableWorker,RemoteCoroutineWorker) subclass is markedprivate, because WorkManager's defaultWorkerFactoryinstantiates workers reflectively and requires apublicclass. D26a should be updated to addListenableWorker/Worker/CoroutineWorkersubclasses to the exception list alongsideActivity,Service,BroadcastReceiver, andTileServiceso they staypublicwithout requiring@SuppressLint("WorkerHasAPublicModifier").
| import java.util.UUID | ||
|
|
||
| private const val PHOTO_UPLOAD_WORK_NAME = "photo_upload" | ||
| @SuppressLint("StaticFieldLeak") |
There was a problem hiding this comment.
Instead of having a context variable and suppressing this lint error, can we pass in context to the method in line 35 (even though it results in a visible code change)?
There was a problem hiding this comment.
We originally had suspend fun updatePhotoUploadWork(context: Context) in commit 13030b9, but audit rule D5 (Smallest Delta / Zero Visible Diff) required reverting the visible function signature to suspend fun updatePhotoUploadWork() and moving context to an out-of-band declaration outside the region tag (commit 30aa6be). Keeping @SuppressLint("StaticFieldLeak") private lateinit var context: Context outside the region tag preserves zero visible diff against the original DAC snippet.
There was a problem hiding this comment.
I think the original way of having context as a parameter looks better in the code, having a Context variable feels a bit hack-y
There was a problem hiding this comment.
Removed @SuppressLint("StaticFieldLeak") private lateinit var context: Context and restored context: Context as a parameter on suspend fun updatePhotoUploadWork(context: Context) in commit 270d61d1, and recorded the signature change in the PR description.
Skill review note for @hamen (android-snippets-experimental):
- Rules impacted: D5 (Permitted Diff Justifications / Minimize Diffs) (
SKILL.mdlines 175–179, 269–271) andcode_review.mdRules 1 & 6 (keeping supporting variables outside region tags to preserve visible function signatures verbatim). - What needs review in the skill: When an original hardcoded DAC snippet includes a function signature inside the code block (
suspend fun updatePhotoUploadWork()) and references an undeclared variable likecontextin its body (WorkManager.getInstance(context)), D5 and Rule 6 require keeping the visible function signature unchanged and declaring supporting variables outside the region tag. However, declaringprivate lateinit var context: Contextat file scope triggers Android Lint'sStaticFieldLeakcheck (requiring@SuppressLint("StaticFieldLeak")) and feels hacky in Kotlin compared to passingcontext: Contextas a function parameter. D5 and Rule 6 should be updated to allow adding missing contextual parameters (such ascontext: Context) to a visible function signature when declaring them as file-level properties would require@SuppressLint("StaticFieldLeak")or introduce an Android anti-pattern.
| // [START android_background_observe_progress_worker] | ||
| // import android.content.Context | ||
| // import androidx.work.CoroutineWorker | ||
| // import androidx.work.Data |
There was a problem hiding this comment.
this import is not needed in the snippet right?
There was a problem hiding this comment.
Removed // import androidx.work.Data in commit 270d61d1 since ProgressWorker uses workDataOf(...) (androidx.work.workDataOf) and never references Data directly.
Skill review note for @hamen (android-snippets-experimental):
- Rules impacted: D18 (Instructional Imports Inside Region Tags) (
SKILL.mdlines 188–199) and D5 (Permitted Diff Justifications) (SKILL.mdlines 175–179). - What needs review in the skill: Under D18, "If an
importstatement appears inside the page's code block, keep it at the exact same relative position inside the region tag as a commented-out import (// import ...), while placing the real compilingimportat the top of the file." In the original hardcoded DAC block onobserve.md,import androidx.work.Datawas present at the top of the snippet block even thoughDatais unused inProgressWorker(which callsworkDataOf(...)). Keeping// import androidx.work.Dataverbatim under D18 / D5 preserves an unused import in the snippet. D18 and D5 should be updated to explicitly state that unused instructional imports carried over from legacy DAC blocks (such asimport androidx.work.DatawhenworkDataOfis used) should be removed rather than commented out with// import ....
| // Retrieve WorkInfo instance. | ||
| val workInfo = workManager.getWorkInfoById(oldWorkRequestId).get() | ||
|
|
||
| // Call getGeneration to retrieve the generation. |
There was a problem hiding this comment.
update this comment since you changed the code?
There was a problem hiding this comment.
Updated the comment on line 69 to // Retrieve the generation. in commit 270d61d1 (and updated the corresponding prose in update-work.md on cl/986196893).
Skill review note for @hamen (android-snippets-experimental):
- Rules impacted: D9 & D29 (Comment Copying, Closed Fill-in List, & Full Stops) (
SKILL.mdlines 165–170) and D5 (Permitted Diff Justifications) (SKILL.mdlines 175–179). - What needs review in the skill: Under D9 / D29, comments must be copied verbatim with only two permitted adjustments: the closed fill-in list (
insert your code here.->Insert your code here) and trailing full stops (.). Because line 70 had to be updated fromworkInfo.getGeneration()toworkInfo?.generationunder D17 so the Kotlin snippet compiles againstandroidx.work.WorkInfo, keeping the original comment// Call getGeneration to retrieve the generation.verbatim under D9 left the comment out of sync with the updated code. D9 and D5 should be updated to add an explicit exception allowing inline comments that name a specific method/API to be updated when the adjacent code line is modified under D2 / D17 / D24.
| val myWorkRequest = ... | ||
| // [START_EXCLUDE silent] | ||
| */ | ||
| val myWorkRequest = OneTimeWorkRequestBuilder<MyWork>().build() |
There was a problem hiding this comment.
I still think we should make the code change because line 43 won't be compiled. So I prefer to reduce the amount of code in comments.
| } | ||
| // [END android_background_long_running_coroutine_worker] | ||
|
|
||
| @SuppressLint("WorkerHasAPublicModifier") |
There was a problem hiding this comment.
I still think we should try doing this (as the private modifier is more of a preference, if it doesn't have any other side effects)
| import java.util.UUID | ||
|
|
||
| private const val PHOTO_UPLOAD_WORK_NAME = "photo_upload" | ||
| @SuppressLint("StaticFieldLeak") |
There was a problem hiding this comment.
I think the original way of having context as a parameter looks better in the code, having a Context variable feels a bit hack-y
kkuan2011
left a comment
kkuan2011
left a comment
There was a problem hiding this comment.
Thanks for the detailed PR descriptions too. To make them more concise for DRE reviewers, could you omit the "Files & Region Tags Added" and "Compile-driven resources" sections so they can dive immediately into the list of diffs?
|
Updated the PR description to omit the "Files & Region Tags Added" and "Compile-Driven Supporting Resources" sections so reviewers can jump straight to the List of modifications. Skill review note for @hamen (
|
This branch has not been deployed
3 participants
Summary
Extracts Kotlin code snippets for 8 persistent background work (WorkManager) documentation guides into
:backgroundwork(backgroundwork/src/main/java/com/example/snippets/backgroundwork/) as region-tagged source code.Pages covered:
List of modifications
CustomConfiguration.kt—android_background_custom_configuration_on_demand): Updatedoverride fun getWorkManagerConfiguration()tooverride val workManagerConfiguration: Configuration get() = ...becauseConfiguration.ProviderdefinesworkManagerConfigurationas a Kotlin property in WorkManager 2.9+.DefineWork.kt—android_background_enqueue_work_request,android_background_expedited_work_request,android_background_assign_input_data):val myWorkRequest = ...inandroid_background_enqueue_work_requestwithval myWorkRequest = OneTimeWorkRequestBuilder<MyWork>().build().<b>,</b>,<var>,</var>), wrapped the hiddenuploadFile(uri: String)stub inUploadWorkinside// [START_EXCLUDE]...// [END_EXCLUDE]so DevSite renders the ellipsis automatically, and keptval myUploadWorkinandroid_background_assign_input_dataat top-level scope so bothclass UploadWorkandval myUploadWorkshare column-0 indentation.LongRunningWorker.kt—android_background_long_running_foreground_service_type): Stripped inline HTML<b>/</b>tags aroundFOREGROUND_SERVICE_TYPE_LOCATION or FOREGROUND_SERVICE_TYPE_MICROPHONE.ObserveWork.kt—android_background_observe_progress_worker): Removed the unusedandroidx.work.Dataimport (ProgressWorkerusesworkDataOf) and preserved the remaining 4 instructional imports (Context,CoroutineWorker,WorkerParameters,delay) inside the region tag as commented imports (// import ...) while placing the compiling imports at the file header.UpdateWork.kt—android_background_update_photo_upload_work,android_background_track_work_generation):context: Contextparameter tosuspend fun updatePhotoUploadWork(context: Context)soWorkManager.getInstance(context)compiles without a file-levelContextvariable.workManager.getWorkInfoById(oldWorkRequestId)toworkManager.getWorkInfoById(oldWorkRequestId).get(), updatedworkInfo.getGeneration()toworkInfo?.generation(sincegetWorkInfoById()returnsListenableFuture<WorkInfo?>andgenerationis a Kotlin property), and updated the preceding comment from// Call getGeneration to retrieve the generation.to// Retrieve the generation..CoroutineWorkerThreading.kt—android_background_coroutine_download_worker_with_context): ChangedwithContext(Dispatchers.IO) { ... return Result.success() }toreturn withContext(Dispatchers.IO) { ... Result.success() }because non-localreturninsidewithContextis prohibited in Kotlin.CustomConfiguration.kt,DefineWork.kt,LongRunningWorker.kt,ManageWork.kt,ObserveWork.kt,UpdateWork.kt): Added trailing periods (.) to natural-language inline comments carried over from DAC.ic_work_notification.xml,strings.xml,AndroidManifest.xml— no region tags): Added Material work notification vector icon (ic_work_notification.xml), string resources (strings.xml), and WorkManager manifest declarations (AndroidManifest.xml) required for the Kotlin snippets to compile and pass:backgroundwork:lintDebug.Snippets not migrated
AndroidManifest.xmlconfiguration snippets (custom-configuration.md,long-running.md,coroutineworker.md,listenableworker.md) remain inline on the DAC pages with{# disableFinding(SNIPPET_GITHUB) #}.Verification
./gradlew :backgroundwork:build- passes (0 errors)./gradlew :backgroundwork:lintDebug- passes (0 errors)./gradlew :backgroundwork:compileDebugKotlin- passes (0 errors)./gradlew :backgroundwork:spotlessApply- formatted cleanly